DeinAlptraum wrote: Thanks a lot for your feedback! Yup I get that the PR is pretty big and might still need significant changes.
> 1. I have maintainability concerns about `ClangLib` protocol [...] I completely agree that this is ugly, but I didn't find a better solution that would enable a strict type check. How do you think we should handle this? 1. Do you know of another solution that would correctly annotate this? It would be nice if we could just reuse the types as described in `functionList` but I don't think this is possible 2. Alternatively, we could `type: ignore` all calls to `ClangLib` attributes. That would require about 180 such annotations I believe. 3. Further, we could go without a strict type-check, only fixing type errors to pass a non-strict typecheck, as well as annotating the user-facing interfaces Regarding the other two points: I tried to change as little as possible here in order to enable type annotations or fix type errors. While there's a lot of places in `cindex.py` that could use refactoring etc. I held off on doing this _unless it was strictly necessary_ for annotations. Both > 2. I see several bugfixes that you highlighted with your comments. I > believe they should be done as a separate PR, because they do something else > than just add typing annotations. and > 3. Changes to enums are massive, and feel somewhat out of place in this > PR as well. were a direct result of my attempt to fix type errors or annotate interfaces correctly. The enum changes were also necessary, since the implementation up to now "dynamically" assigned the enum variants after declaration of the class. That means if you use e.g. `CursorKind.UNEXPOSED_DECL` in a script, this will work fine but fail the type check since it doesn't recognize `UNEXPOSED_DECL` as an attribute of `CursorKind`. This is thus effectively part of annotating the user-facing interfaces. Should I close this PR for now and split this into multiple PRs for the bugfixes and then several smaller, grouped changes? https://github.com/llvm/llvm-project/pull/78114 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
