-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix type errors #105
Fix type errors #105
Conversation
In my opinion, the remaining errors/warnings (when setting
@hofaflo let me know if you think we should address any of the remaining errors (and if so, please show me how). |
Ima turn off type checking now to stay sane. |
d3266d6
to
cc9a7fd
Compare
Pyright will still complain, but I think making mypy happy is enough :) The mkdocstrings issue is already fixed upstream (mkdocstrings/python#45), so I bumped the dependency. 5e81986 should have worked around this, but for some reason readthedocs ignored the changed path to the configuration file and kept calling mkdocs with |
This is ready to merge from my point of view @cbrnr :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks better than expected 😄! I have a couple of questions (inline) plus the following two points:
- Regarding the
py.typed
file – why is this empty file necessary (what does it do)? And if it is necessary, should it not be included aspackage_data
? - I think we should add
.mypy_cache
to.gitignore
.
@hofaflo, besides my minor comments this looks really good (I've edited my comments so that all questions are in the first comment). |
PEP561 says so 🤷. I'm not totally sure but I assume it tells type checkers (such as mypy) that the package is typed.
Totally, it is included in the wheels and in the source archive (I guess because its parent directory is listed in MANIFEST.in?).
For some reason my VS Code already ignored it so I didn't notice, but sure, I'll add it :D |
Looks great! Ready to merge? |
Yes! |
Thanks @hofaflo! |
Fixes #93.