Add type annotations #123

cj81499 · 2023-08-29T23:56:51Z

Apologies for the awful commit history. We'll want to squash merge (or rebase it into a single commit before merge).

resolves #78

browser_cookie3 does not have type hints available

codecov · 2023-08-29T23:58:07Z

Codecov Report

Attention: 22 lines in your changes are missing coverage. Please review.

Comparison is base (c94c844) 91.63% compared to head (c61b3e3) 91.71%.
Report is 7 commits behind head on main.

❗ Current head c61b3e3 differs from pull request most recent head db4fe6a. Consider uploading reports for the commit db4fe6a to get more accurate results

Files	Patch %	Lines
aocd/_ipykernel.py	15.38%	11 Missing ⚠️
aocd/models.py	95.90%	3 Missing and 2 partials ⚠️
aocd/cookies.py	50.00%	4 Missing ⚠️
aocd/utils.py	93.93%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #123      +/-   ##
==========================================
+ Coverage   91.63%   91.71%   +0.08%     
==========================================
  Files          24       25       +1     
  Lines        2653     2739      +86     
  Branches      356      362       +6     
==========================================
+ Hits         2431     2512      +81     
- Misses        158      163       +5     
  Partials       64       64

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

aocd/__init__.py

aocd/_ipykernel.py

aocd/examples.py

aocd/models.py

pyproject.toml

.gitignore

wimglenn · 2023-11-28T02:43:14Z

tests/test_submit.py

    post = pook.post(url="https://adventofcode.com/2018/day/1/answer")
    submit(1234, part="a", day=1, year=2018, session="whatever", reopen=False)
    assert post.calls == 0


-def test_submit_float_warns(pook, capsys, caplog):
+def test_submit_float_warns(pook: pook_mod, caplog: pytest.LogCaptureFixture) -> None:


@cj81499 I don't understand why pook fixture (which happens to also yield the pook module) is annotated as the module itself. Why shouldn't it be annotated as types.ModuleType?

wimglenn · 2023-11-28T03:04:41Z

tests/test_auth.py

    test_token.unlink()
-    with pytest.raises(AocdError("Missing session ID")):
+    with pytest.raises(AocdError("Missing session ID")): # type: ignore[call-overload] # using pytest-raisin


There are 41 occurences of this pragma added

# type: ignore[call-overload] # using pytest-raisin

Is there a way to just globally disable the type of overload which pytest-raisin does in mypy configuration or something? It adds a lot of noise.

Alternatively, is there some annotation that can be made in the plugin pytest-raisin directly to help mypy understand that pytest_raisin.raises is a wrapper around pytest.raises, and it has extended the supported types in the call signature?

wimglenn · 2023-11-28T03:16:52Z

aocd/models.py

+        original_val = val
+        def fail() -> NoReturn:
+            msg = f"Failed to coerce {type(original_val).__name__} value {original_val!r} for {self.year}/{self.day:02}."
+            raise AocdError(msg)


fail is only used once. Is there a good reason for defining a nested function here, instead of just raising directly inline?

tests/test_models.py

aocd/models.py

wimglenn · 2023-11-28T03:27:38Z

aocd/models.py

+    class _Results(TypedDict):
+        part: _Part
+        value: str
+        when: str
+        message: str


should this be moved into _types.py?

wimglenn · 2023-11-28T03:27:57Z

aocd/models.py

+class _SolverCallable(Protocol):
+    def __call__(self, year: int, day:int, data: str) -> _Answer:
+        ...
+
+_ExampleParserCallable = Callable[[_examples.Page, list[str]], list[_examples.Example]]
+
+class _Result(TypedDict):
+    time: timedelta
+    rank: int
+    score: int


should these be moved into _types.py?

wimglenn · 2023-11-28T04:01:32Z

aocd/get.py

        raise giveup("Failed introspection of year")
-    year = years.pop() if years else None
+    year = years.pop()


Unfortunately this stricter parsing is no bueno - I have it written in the README here that a filename like q03.py would work, and it works by returning year=None then letting some later code check for None and call most_recent_year() instead.

Even though that behavior was questionable (the aocd introspection will only work currently when run within the same year as the event, and those scripts will start to break a week after the event finishes), I think it's best to keep this behavior, because it means you can omit mentioning the current year in your filenames entirely. I'll bet a lot of new users / people playing AoC for the first time (during the event) are doing just that.

So, let's go with something along these lines:

if len(years) > 1: raise giveup("Failed introspection of year") elif len(years) == 1: [year] = years else: year = get_current_year() log.debug(f"year could not be determined, assuming current year ({year})")

mjpieters

In general, I prefer using from __future__ import annotations and then use more modern syntax in type annotations such as | for unions and optional values. This is a lot more readable than using Union and Optional everywhere. See the typing best practices documentation.

I'd also break this PR up into at least 2, preferably more PRs, where you introduce typing more gradually. This is a huge PR to review and that makes it all the harder for the maintainer to review and accept this.

E.g. you could move type annotations for the tests to a separate PR, and focus on just the public APIs and adding py.typed first in a first PR. Or, just use mine (#131) for that first step ;-)

aocd/__init__.py

mjpieters · 2023-12-07T16:18:19Z

aocd/_types.py

+    "_LoosePart",
+]
+
+_Answer = Optional[Union[str, bytes, int, float, complex, Fraction, Decimal, "np.number[np.typing.NBitBase]"]]


All the number types are (virtual) subclasses of numbers.Number, there is no need to pull in numpy for this:

from decimal import Decimal from fractions import Fraction from numbers import Number import numpy as np for tp in (int, float, complex, Fraction, Decimal, np.int_, np.float_, np.complex_): print(tp, issubclass(tp, Number)) # output: # <class 'int'> True # <class 'float'> True # <class 'complex'> True # <class 'fractions.Fraction'> True # <class 'decimal.Decimal'> True # <class 'numpy.int64'> True # <class 'numpy.float64'> True # <class 'numpy.complex128'> True

Even if this wasn't the case, I'd have used np.number[Any], as it doesn't matter to aocd what type of generic argument np.number[] accepts.

oh, nice 👍

mjpieters · 2023-12-07T16:20:28Z

aocd/_types.py

+if TYPE_CHECKING:
+    import numpy as np
+
+__all__ = [


Why give these objects private names and then export them? The types would be useful for consumers of the library too; e.g. some 3rd party library might use:

from aocd.types import Answer, Part def process_answer(answer: Answer, part: Part | None = None, **kwargs: Any) -> None: """Process an AOC answer before submitting""" ...

and you may want to store some of these types:

from aocd.types import Answer # puzzle is a aocd.models.Puzzle instance. answers: tuple[Answer, Answer] = puzzle.solve()

Note that the aocd._types module is already itself private. The names in the module should then be public; their 'visibility' then extends only to the aocd package and no further. Unless you explicitly export them, of course.

mjpieters · 2023-12-07T16:25:44Z

aocd/models.py

+    def get_stats(
+        self,
+        years: Optional[Union[Iterable[int], int]] = None
+    ) -> dict[str, dict[Literal['a', 'b'], _Result]]:


Why wasn't the _types._Part type alias used here, instead of Literal['a', 'b']?

mjpieters · 2023-12-07T16:28:13Z

aocd/models.py

-        if isinstance(years, int) and years in all_years:
+        if isinstance(years, int):
+            if years not in all_years:
+                raise ValueError(f"Valid years are {all_years.start} through {all_years.stop - 1}. Got {years}.")


I had found this too, which is why type checking is so valuable! I was going to raise a separate issue about this one :-P

It'd be better if there was type validation for any years argument passed in however, not just when it's a single year. E.g. user.get_stats((2013, 2014)) is just as wrong as user.get_stats(2014).

mjpieters · 2023-12-07T18:18:07Z

aocd/runner.py

+    progress: Optional[str],
+    dt: float = 0.1,
+    capture: bool = False,
+    **kwargs: object


Do not use object here, this is definitely a place to use Any.

Suggested change

**kwargs: object

**kwargs: Any

See the typing best practices documentation on Any vs. object. Here kwargs are keyword arguments passed to _timeout_wrapper and you can't express what types are acceptable to the function that is obtained by entry_point.load() inside this function.

mjpieters · 2023-12-07T18:25:58Z

aocd/runner.py

-    reopen=False,
-    capture=False,
-):
+    plugs: Iterable[str],


Plugs can't be an iterable, because it needs to support containment and iteration.

Using plugs: Iterable[str] here is wrong because you can't just pass in a generator yielding plugs to this function and expect it to work. Generators are valid iterables (they implement __iter__), but using a generator would lead to unexpected side effects. Further down, the line eps = {ep.name: ep for ep in get_plugins() if ep.name in plugs} uses a containment test on plugs, and if plugs is a generator then the in test would iterate over the values until it found a match. And that in turn leads to the itertools.product() function being passed a partially or fully iterated plugs iterator. Try passing in (ep.name for ep in get_plugins()) for example; all the plugins would end up being skipped

Use typing.Collection[str] instead:

Suggested change

plugs: Iterable[str],

plugs: Collection[str],

Collections are iterable and support containment directly. Generators are not collections.

aocd/utils.py

mjpieters · 2023-12-07T18:46:39Z

pyproject.toml

-classifiers = [
+classifiers = [  # https://pypi.org/classifiers/
    "Intended Audience :: Developers",
-    "Programming Language :: Python :: 3",
    "License :: OSI Approved :: MIT License",
-    "Topic :: Software Development :: Libraries",
+    "Programming Language :: Python :: 3",
    "Topic :: Games/Entertainment :: Puzzle Games",
+    "Topic :: Software Development :: Libraries",


These changes are not really part of the type hinting goal, are they?

mjpieters · 2023-12-07T18:47:24Z

pyproject.toml

    "aocd-example-parser >= 2023.2",
+    'typing-extensions; python_version < "3.11"',


As stated above, I'd just make this unconditional. typing_extensions is basically part of the reality of Python libraries now.

The reason for this was that typing.Self was not avail in stdlib until 3.11.

I know why typing_extensions is used. :-) My point is that the whole dance in _compat plus the condition on python_version < "3.11" here is overkill.

It is far simpler to just use from typing_extensions import Self in your code and to install typing_extensions unconditionally. aocd is far from the only library that would have a dependency ontyping_extensions, in any given Python version, at the moment.

Co-authored-by: Martijn Pieters <[email protected]>

wimglenn · 2023-12-07T19:48:49Z

Thank you for the in-depth review, @mjpieters, a lot of good suggestions here!

wimglenn · 2024-08-09T03:32:59Z

@cj81499 Have you lost interest in this PR? There are many conflicts now, but if it's alright with you I'll likely use some of the ideas from the PR - particularly around the refactoring of the submission types.

cj81499 · 2024-08-10T14:51:21Z

I wouldn't say I've lost interest completely, but I have a hard time making time to contribute to OSS, and my motivation is lower when I'm not actively using the library during (or shortly after) December.

You're free to close this PR and pick out the parts you think are valuable. I would appreciate a "Co-authored-by: Cal Jacobson [email protected]" at the end of the commit if you use my work in that way.

wimglenn · 2024-11-03T01:00:54Z

Replaced with 8422243 and released in v2.0.2.

cj81499 · 2024-11-03T02:40:24Z

Thanks @wimglenn!

cj81499 added 21 commits August 29, 2023 18:27

add .gitignore

4bb3f90

list testing dependencies in pyproject.toml

226ed10

add type annotations

d0300bb

wip

9001f79

wip

42beb06

wip

0be9870

udpate gitignore

5833326

enable ignore_missing_imports for browser_cookie3

2cdd669

browser_cookie3 does not have type hints available

update trove classifiers

f79fa36

ignore bc3

6ba0c82

improve coverage report exclusion rules

d364823

wip

39ea8c1

wip

803c2fc

wip

c782e2f

wip

85e3e8b

wip

eae8d2e

wip

5749296

fix pyproject.toml oops

bea135f

wip

d1dfc65

wip

aca3c5d

wip

840a196

cj81499 added 3 commits August 29, 2023 19:07

wip

288920f

wip

60dcb44

improve coverage exclusion rules

c61b3e3

cj81499 commented Aug 30, 2023

View reviewed changes

aocd/__init__.py Outdated Show resolved Hide resolved

aocd/_ipykernel.py Show resolved Hide resolved

aocd/examples.py Outdated Show resolved Hide resolved

aocd/models.py Outdated Show resolved Hide resolved

pyproject.toml Outdated Show resolved Hide resolved

wimglenn reviewed Aug 30, 2023

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

cj81499 mentioned this pull request Sep 8, 2023

Add type annotations cj81499/advent-of-code-data#1

Closed

cj81499 added 2 commits November 6, 2023 17:41

python 3.12

a75a223

revert add .gitignore

0cfcfba

cj81499 and others added 10 commits November 26, 2023 12:22

revert example parser regression

dad87b0

refactor: extract version dependent logic to _compat module

408c14e

remove classifiers

2bf5021

remove unused imports

0d87e5b

use Self instead of _TUser

d3ea107

refactor compat module

8bd78b0

run through https://github.com/asottile/reorder-python-imports

44e9635

rework coercion

0cc874c

support bytes, Fraction, and Decimal types

54dcec4

add comments to coercion test cases

5747886

wimglenn reviewed Nov 28, 2023

View reviewed changes

tests/test_models.py Show resolved Hide resolved

wimglenn reviewed Nov 28, 2023

View reviewed changes

aocd/models.py Show resolved Hide resolved

wimglenn reviewed Nov 28, 2023

View reviewed changes

mjpieters mentioned this pull request Dec 7, 2023

Provide type annotations for the public API #131

Merged

mjpieters reviewed Dec 7, 2023

View reviewed changes

wimglenn and others added 3 commits December 7, 2023 13:12

Update aocd/utils.py

bd15325

Co-authored-by: Martijn Pieters <[email protected]>

Update aocd/models.py

495519e

Co-authored-by: Martijn Pieters <[email protected]>

Update aocd/models.py

db4fe6a

Co-authored-by: Martijn Pieters <[email protected]>

wimglenn force-pushed the main branch from 0499632 to 8422243 Compare November 3, 2024 00:38

wimglenn closed this Nov 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add type annotations #123

Add type annotations #123

cj81499 commented Aug 29, 2023 •

edited

Loading

codecov bot commented Aug 29, 2023 •

edited

Loading

wimglenn Nov 28, 2023

wimglenn Nov 28, 2023 •

edited

Loading

wimglenn Nov 28, 2023

wimglenn Nov 28, 2023

wimglenn Nov 28, 2023

wimglenn Nov 28, 2023 •

edited

Loading

mjpieters left a comment

mjpieters Dec 7, 2023

wimglenn Dec 7, 2023

mjpieters Dec 7, 2023 •

edited

Loading

mjpieters Dec 7, 2023

mjpieters Dec 7, 2023

mjpieters Dec 7, 2023

mjpieters Dec 7, 2023 •

edited

Loading

mjpieters Dec 7, 2023

mjpieters Dec 7, 2023

wimglenn Dec 7, 2023

mjpieters Dec 7, 2023

wimglenn commented Dec 7, 2023

wimglenn commented Aug 9, 2024

cj81499 commented Aug 10, 2024

wimglenn commented Nov 3, 2024

cj81499 commented Nov 3, 2024

		"aocd-example-parser >= 2023.2",
		'typing-extensions; python_version < "3.11"',

Add type annotations #123

Add type annotations #123

Conversation

cj81499 commented Aug 29, 2023 • edited Loading

codecov bot commented Aug 29, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

wimglenn Nov 28, 2023 • edited Loading

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

wimglenn Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

mjpieters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjpieters Dec 7, 2023 • edited Loading

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 7, 2023 • edited Loading

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

wimglenn commented Dec 7, 2023

wimglenn commented Aug 9, 2024

cj81499 commented Aug 10, 2024

wimglenn commented Nov 3, 2024

cj81499 commented Nov 3, 2024

cj81499 commented Aug 29, 2023 •

edited

Loading

codecov bot commented Aug 29, 2023 •

edited

Loading

wimglenn Nov 28, 2023 •

edited

Loading

wimglenn Nov 28, 2023 •

edited

Loading

mjpieters Dec 7, 2023 •

edited

Loading

mjpieters Dec 7, 2023 •

edited

Loading