Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 annotations #123
Add type annotations #123
Changes from all commits
4bb3f90
226ed10
d0300bb
9001f79
42beb06
0be9870
5833326
2cdd669
f79fa36
6ba0c82
d364823
39ea8c1
803c2fc
c782e2f
85e3e8b
eae8d2e
5749296
bea135f
d1dfc65
aca3c5d
840a196
288920f
60dcb44
c61b3e3
a75a223
0cfcfba
b66dc7f
9fc0e29
d15e818
7717874
622e783
fa37a44
766807b
14d4adf
bf9e19a
58198a2
dad87b0
408c14e
2bf5021
0d87e5b
d3ea107
8bd78b0
44e9635
0cc874c
54dcec4
5747886
bd15325
495519e
db4fe6a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Just use
Any
. The correct type hints are already provided for all names in__all__
so the type checker doesn't have to care about what__getattr__
returns.(make sure to import
Any
).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 is overkill. Just use
from typing_extensions import Self
, where you need it in the modules itself, and unconditionally depend ontyping_extensions
.typing_extensions
should be kept as a dependency even on 3.11 because it is, by now, used by a lot of projects so is not likely to be a new addition.typing_extensions
itself will usetyping.Self
on Python 3.11 so checking for the Python version here won't buy you anything.advent-of-code could instead use pyupgrade to keep the codebase clean; if in future the minimum release for the project is moved to Python 3.11, that tool would automatically move the import from
typing_extensions
totyping
.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.
Interesting. As well as moving the import, is
pyupgrade
also smart enough to know when thepyproject.toml
metadata no longer needs to specify a dependency on typing-extensions?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.
pyupgrade won't (dependency management is out of scope), but a tool like deptry can be used to flag obsolete dependencies (as well as flag missing deps).
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.
Ditto here. Just use
from typing_extensions import ParamSpec
in the module that needs to use aParamSpec
annotation.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.
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:
and you may want to store some of these types:
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 theaocd
package and no further. Unless you explicitly export them, of course.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.
All the number types are (virtual) subclasses of
numbers.Number
, there is no need to pull in numpy for this:Even if this wasn't the case, I'd have used
np.number[Any]
, as it doesn't matter toaocd
what type of generic argumentnp.number[]
accepts.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.
oh, nice 👍
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 have my reservations about the use of
_LoosePart
(voiced below), but if you are going to use it, then at least also includeA
andB
; the code that accepts_LoosePart
uses case folding on strings.