Provide type annotations for the public API #131

mjpieters · 2023-12-07T15:48:39Z

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.

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 readable A | B union syntax can be used and have the project still work on Python version 3.9. The exception are the type aliases in aocd.types; Python 3.9 has no typing.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 the typing_extensions library, but that seems overkill for one missing typing 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 in aocd/__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 and TypedDict objects for types that I think consumers of aocd might find useful; e.g. a consumer might want to store a returned PuzzleStats value, and a 3rd-party library might want to expose a method that passes through a PuzzlePart and AnswerValue argument to aocd.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 and aocd.examples.Example named tuple already used type annotations for their fields, but these did not properly annotate the optional fields (those that can be None 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-commitconfiguration that checks for this locally on every commit. See https://github.com/RobertCraigie/pyright-python#pre-commit for what that looks like.

Resolves #78

codecov · 2023-12-07T15:55:09Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (34d73e3) 91.56% compared to head (589325a) 91.72%.
Report is 3 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

mjpieters · 2023-12-07T16:12:34Z

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.

mjpieters · 2023-12-07T16:23:07Z

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.

mjpieters · 2023-12-07T21:27:36Z

aocd/examples.py

-log = logging.getLogger(__name__)
+log: logging.Logger = logging.getLogger(__name__)


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.

mjpieters · 2023-12-07T21:33:34Z

aocd/utils.py

+from __future__ import annotations
+


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.

wimglenn

Looks good, just a few questions/comments

.github/workflows/tests.yml

aocd/utils.py

wimglenn · 2023-12-07T21:21:10Z

.github/workflows/tests.yml

+
+    steps:
+      - uses: actions/checkout@v3
+      - uses: actions/setup-python@v4


What Python version is this going to use by default?

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'.

wimglenn · 2023-12-07T21:21:14Z

.github/workflows/tests.yml

+          # 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


What does this do?

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.

aocd/__init__.py

wimglenn · 2023-12-07T21:47:26Z

aocd/examples.py


    @property
-    def answers(self):
+    def answers(self) -> tuple[str | None, str | None]:


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.

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).

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.

aocd/get.py

wimglenn · 2023-12-17T02:00:36Z

@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?

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`.

mjpieters force-pushed the type_annotations branch from ff024a9 to 184b71e Compare December 7, 2023 15:53

mjpieters force-pushed the type_annotations branch 2 times, most recently from 85f1652 to cd5dce5 Compare December 7, 2023 16:10

mjpieters force-pushed the type_annotations branch from cd5dce5 to 5bd779a Compare December 7, 2023 16:13

mjpieters force-pushed the type_annotations branch 6 times, most recently from af4900f to 87eeac9 Compare December 7, 2023 18:06

mjpieters mentioned this pull request Dec 7, 2023

Add type annotations #123

Open

mjpieters commented Dec 7, 2023

View reviewed changes

wimglenn reviewed Dec 7, 2023

View reviewed changes

mjpieters force-pushed the type_annotations branch 2 times, most recently from abed4c6 to 8981fc6 Compare December 21, 2023 09:44

mjpieters requested a review from wimglenn December 21, 2023 09:45

mjpieters added 2 commits December 21, 2023 09:46

CI: Verify public API types with pyright

589325a

mjpieters force-pushed the type_annotations branch from 8981fc6 to 589325a Compare December 21, 2023 09:47

wimglenn merged commit 40d1ef7 into wimglenn:main Dec 23, 2023
10 checks passed

mjpieters deleted the type_annotations branch December 23, 2023 11:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide type annotations for the public API #131

Provide type annotations for the public API #131

mjpieters commented Dec 7, 2023 •

edited

Loading

codecov bot commented Dec 7, 2023 •

edited

Loading

mjpieters commented Dec 7, 2023

mjpieters commented Dec 7, 2023

mjpieters Dec 7, 2023

mjpieters Dec 7, 2023

wimglenn left a comment

wimglenn Dec 7, 2023

mjpieters Dec 21, 2023

wimglenn Dec 7, 2023

mjpieters Dec 21, 2023

wimglenn Dec 7, 2023

mjpieters Dec 21, 2023 •

edited

Loading

mjpieters Dec 21, 2023

wimglenn commented Dec 17, 2023

		log = logging.getLogger(__name__)
		log: logging.Logger = logging.getLogger(__name__)

Provide type annotations for the public API #131

Provide type annotations for the public API #131

Conversation

mjpieters commented Dec 7, 2023 • edited Loading

codecov bot commented Dec 7, 2023 • edited Loading

Codecov Report

mjpieters commented Dec 7, 2023

mjpieters commented Dec 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wimglenn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjpieters Dec 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wimglenn commented Dec 17, 2023

mjpieters commented Dec 7, 2023 •

edited

Loading

codecov bot commented Dec 7, 2023 •

edited

Loading

mjpieters Dec 21, 2023 •

edited

Loading