-
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
Provide type annotations for the public API #131
Conversation
ff024a9
to
184b71e
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #131 +/- ##
==========================================
+ Coverage 91.56% 91.72% +0.15%
==========================================
Files 24 25 +1
Lines 2656 2706 +50
Branches 357 358 +1
==========================================
+ Hits 2432 2482 +50
Misses 159 159
Partials 65 65 ☔ View full report in Codecov by Sentry. |
85f1652
to
cd5dce5
Compare
I managed to completely miss the other PR here, #123. I'll take a look to see what is different between mine and that one. |
cd5dce5
to
5bd779a
Compare
The difference between this PR and #123 is that this PR is much more limited in scope. It only aims to provide type annotations for consumers of the library, not for the library itself. See it as a multi-step process; type hinting for internal use could be added separately. This makes this PR a lot smaller. |
af4900f
to
87eeac9
Compare
log = logging.getLogger(__name__) | ||
log: logging.Logger = logging.getLogger(__name__) |
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.
The log
objects in each module should probably be private, so _log
. The only reason I annotated them is because they are part of the public API.
from __future__ import annotations | ||
|
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.
To be honest, I suspect that the whole utils
module should really be named _utils
, and not be part of the public API at all.
But that's up to the maintainer of the project, really.
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 good, just a few questions/comments
|
||
steps: | ||
- uses: actions/checkout@v3 | ||
- uses: actions/setup-python@v4 |
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.
What Python version is this going to use by default?
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.
Good point; it should really pick a version. It'll use the value from .python-version
if that file is present, but if not it'll default to the 'default Python installed on the runner'.
# https://microsoft.github.io/pyright/#/import-resolution?id=editable-installs | ||
pip install --editable . --config-settings editable_mode=strict | ||
|
||
- run: echo "$PWD/.venv/bin" >> $GITHUB_PATH |
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.
What does this do?
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.
This puts the venv on the PATH for all following steps. It lets pyright find the venv as it looks for the current python executable.
|
||
@property | ||
def answers(self): | ||
def answers(self) -> tuple[str | None, str | None]: |
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.
What is the code-path where these can be None
? IIRC we raise exception for unavailable answers, and day 25 part b answer is always the empty string.
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.
For the Example
named tuple, the default values for answer_a
and answer_b
is None
. Your code creates a missing = Example("")
instance, and missing.answers
is (None, None)
.
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 think you were thinking of Puzzle.answers
here, which can only return tuple[str, str]
and will raise an exception if that's not possible. But anyone can create an Example
instance with one or other answer ommitted.
@mjpieters I will likely merge this PR first and then rebase the other one on your changes. Could you address/resolve these review comments first, though? |
abed4c6
to
8981fc6
Compare
The package now includes a PEP 561 `py.typed` flag file to let type checkers know that this package provides type annotations. The annotations provide full coverage for all public functions and classes in the aocd package, with the library reaching 100% coverage as per `pyright --ignoreexternal --verifytypes`.
8981fc6
to
589325a
Compare
The package now includes a PEP 561
py.typed
flag file to let typecheckers know that this package provides type annotations.
The annotations provide full coverage for all public functions and
classes in the aocd package, with the library reaching 100% coverage as
per
pyright --ignoreexternal --verifytypes
.A few notes on this PR:
I tried to follow the current best practices for type annotations as much as possible. This includes using
from __future__ import annotations
so the newer but much more readableA | B
union syntax can be used and have the project still work on Python version 3.9. The exception are the type aliases inaocd.types
; Python 3.9 has notyping.TypeAlias
and you can't use|
syntax in a type alias assignment in 3.9 (it's not an annotation). The first issue could be solved by adding a dependency on thetyping_extensions
library, but that seems overkill for one missingtyping
feature.The PR is limited to the public API members only. All private names are left untouched. The goal was to provide consumers of aocd with type annotations, not to validate the types used within aocd itself. Contrast this with Add type annotations #123 which tries to provide full typing coverage of the whole codebase.
I re-introduced the
__all__
list inaocd/__init__.py
because that's what type checkers use to determine what is public more than anything else. Without this list, none of the imports in__init__.py
would be seen as exported. I made sure to only export what was exported before.A new
aocd.types
module holds public type aliases andTypedDict
objects for types that I think consumers ofaocd
might find useful; e.g. a consumer might want to store a returnedPuzzleStats
value, and a 3rd-party library might want to expose a method that passes through aPuzzlePart
andAnswerValue
argument toaocd.submit()
. Plus, these provide inline documentation in IDEs that make use of type information (e.g. PyCharm or Visual Studio Code with the Pylance extension).The
aocd.examples.Page
dataclass andaocd.examples.Example
named tuple already used type annotations for their fields, but these did not properly annotate the optional fields (those that can beNone
as well as their intended type). Changing these to... | None
means their API has now changed. I don't think this is a big deal, it was really a bug and so I doubt this needs a deprecation warning.A second commit extends the Github workflow to validate that the public API keeps the 100% coverage. I recommend adding a
pre-commit
configuration that checks for this locally on every commit. See https://github.com/RobertCraigie/pyright-python#pre-commit for what that looks like.Resolves #78