-
Notifications
You must be signed in to change notification settings - Fork 57
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
add type stubs #79
add type stubs #79
Conversation
Codecov Report
@@ Coverage Diff @@
## main #79 +/- ##
==========================================
+ Coverage 92.53% 92.57% +0.04%
==========================================
Files 24 24
Lines 2009 2020 +11
Branches 223 264 +41
==========================================
+ Hits 1859 1870 +11
Misses 123 123
Partials 27 27
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
f252390
to
979d766
Compare
"AOC_TZ", | ||
"__version__", | ||
"data", | ||
] |
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.
I'm not too familiar with .pyi files, but does it mean I now need to maintain this list in 2 different places? That's a little annoying..
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.
Unfortunately, yes.
Aside from types directly in .py
files (which is unpleasant when you need Python 2 support, and (afaik) not possible when you're "hacking" sys.modules
like aocd/__init__.py
does, I don't know of anything better.
- "3.11" # newest supported Python 3 | ||
- "pypy-2.7" # PyPy 2 | ||
- "pypy-3.7" # oldest supported PyPy 3 | ||
- "pypy-3.9" # newest supported PyPy 3 | ||
|
||
steps: |
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.
I'm not super familiar with typing, but can you add something in here which actually uses mypy? So that we are getting some value and "coverage" of the stubs directly in the CI.
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.
That should be doable.
The .py
files in aocd
are ignored by mypy
because the stubs exist, so perhaps we should check that the tests use the type hints correctly?
I guess the other option would be to use the comment form of type hints directly within the .py
files (except for __init__.py
, where we can't (or at least, I don't know how to) due to the sys.modules
hack), and then we could type check both the library and the tests (although, this'll be a larger change...).
There's also stubtest
, although I've never used it. All the other Python I've worked on using types is Python 3.5+, so I can just use the variable_name: type
form in .py
files (rather than .pyi
stubs) so I've just type checked the library and tests directly with mypy
.
It's also worth mentioning, mypy
has fully dropped support for Python <= 3.5. Maybe we should only type check using Python 3.11 (at least, until you decide to drop support for older Python versions)?
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.
Yeah, it's fine to check only on one Python version & platform - whichever version is most convenient. Checking the tests seems OK, although I wouldn't be surprised if something in the mocking is not exactly type-correct (I am only considering "duck types" at best when writing test code)
@cj81499 Support for Python < 3.9 was dropped in #108. The next release will be aocd 2.0.0, and it will require Python >= 3.9. Stub files are no longer the right approach for adding typing, so I'm going to close this PR, but I'll leave #78 open in case you're interested to annotate in the .py directly. |
Makes sense to me. I honestly completely forgot I had this half-done 😵💫 Any idea when you'll release 2.0.0? Would like to understand if I have any chance of putting up a PR in time for release. |
don’t know - sometime before December 1st |
2.0.0 is released now, but that doesn't mean I won't release any new version(s) before Dec, so don't let it discourage you. |
resolves #78