-
-
Notifications
You must be signed in to change notification settings - Fork 82
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
Complete type annotations #342
Conversation
da7146c
to
4eb1aa1
Compare
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 overall looks good, but I think it would be better to replace specific data types like List
and Set
with Iterable
or Collection
in the public API (eg. packages_distributions
, files
, etc).
FYI, I noticed that we weren't pulling |
@@ -1023,6 +1023,6 @@ def _top_level_declared(dist): | |||
def _top_level_inferred(dist): | |||
return { | |||
f.parts[0] if len(f.parts) > 1 else f.with_suffix('').name | |||
for f in dist.files | |||
for f in dist.files or [] |
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 approach would have been my instinct.
Since the docs for .files
suggests to use always_iterable
, I'd prefer to follow that recommendation. I'll explore that option and then decide how to proceed.
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. I see this change combines two changes (adding annotations and fixing the bug for #344). These two concerns should be separate. I recommend to remove these bugfixes from this PR so that it focuses on the type 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.
I already separated each functional change into separate atomic commits. They cannot be removed from this PR, because then it won’t pass mypy—the point of mypy is to find bugs like this!
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 had to remove always_iterable
in favor of or []
because the behavior of always_iterable
is too dynamic to be correctly annotated in Python’s type system. The behavior where it converts a non-iterable input to a one-item iterator cannot be expressed; there’s an Iterable
constraint but not a “non-iterable” constraint. Users who don’t need types are still free to use always_iterable
, of course.
(Again, I’ve done this in a separate commit.)
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 had to remove
always_iterable
...
That's a shame and feels like a defect or weakness in the typing system. I'd like to see a solution that doesn't require changing the implementation. The always_iterable
idiom is a simple yet powerful abstraction and replaces with a single concise expression the common idiom of:
def block(param):
if param is None:
param = []
if isinstance(param, (str, bytes)):
param = [param]
# ... do stuff with iterable of param
If the type system can't support that level of dynamism, then the type system should be avoided until it can.
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.
If that were all always_iterable
did, it would be completely fine:
from typing import Iterable, Iterator, Optional, TypeVar
T = TypeVar("T")
def always_iterable(obj: Optional[Iterable[T]]) -> Iterator[T]:
if obj is None:
return iter(())
if isinstance(obj, (str, bytes)):
return iter((obj,))
return iter(obj)
But the current version has two extra features that interfere with static typing:
def always_iterable(obj, base_type=(str, bytes)):
if obj is None:
return iter(())
if (base_type is not None) and isinstance(obj, base_type):
return iter((obj,))
try:
return iter(obj)
except TypeError:
return iter((obj,))
Firstly, a non-default base_type
might change the behavior so that obj: Optional[Iterable[T]]
does not necessarily imply always_iterable(obj): Iterator[T]
.
Secondly, the TypeError
handler means that one should also have always_iterable(obj): Iterator[T]
when obj: T
—but only if T
is not itself iterable. That extra non-iterable condition is not expressible in mypy, for the fundamental reason that it would break the Liskov substitution principle (e.g. every type is supposed to be a subtype of object
, which is non-iterable). It’s also a fragile condition to rely on even when no annotations are involved (what if T
is updated in the future to become iterable?).
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 see. I had simplified the summary of always_iterable
to focus on the implementation for how it's used/recommended here. You're right that the implementation also addresses the concern of transforming a single item to an iterator over one item.
It solves the common pattern where a function accepts both a single item or an iterable of items but intends to iterate over them. For example consider this change, where always_iterable
transforms the branchy logic into a simple dict comprehension.
That extra non-iterable condition is not expressible in mypy.
That seems like a limitation of mypy. It seems to me that it should be possible to express any valid Python in mypy, especially something as simple as accepting an iterable or non-iterable. Is it possible the level of expressiveness you're trying to achieve is too great? I notice that always_iterable
does already have a type specification. Would that spec suffice or is it too generic for strict mode?
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.
It’s also a fragile condition to rely on
I think the same could be said for or []
, which is also fragile based on the value of ob.__bool__
. If an object were to return False
for __bool__
but not implement iterable, it also would fail. I've found that for all (most?) practical cases, iterability is stable for the uses of any particular function, despite being unreliable in the general case.
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 seems like a limitation of mypy.
Yes. But there’s a good reason for this limitation, since it’s a consequence of the more important Liskov substitution principle: wherever an expression of type T
is expected, it should be type-correct to provide a value whose type is a subclass of T
.
Remember, every non-iterable type might be subclassed by an iterable type (this is not an uncommon corner case: every iterable type is a subclass of object
). So if there were a hypothetical syntax that purported to guarantee that always_iterable(obj): T
when obj: T
and T
is non-iterable, one could assign an instance of an iterable subclass U
to obj: T
and get back a result from always_iterable(obj)
that breaks this guarantee.
A simpler way to say this is that there’s no such thing as a non-iterable type, provided that one recognizes a type to implicitly include all of its subclasses. Giving up that provision would have unpleasant ripple effects on every other aspect of the type system (I think you would basically need to augment each type annotation with a flag saying whether or not a subclass is allowed).
I notice that always_iterable does already have a type specification.
Any
is never a type specification; it just disables type checking for that value and every value derived from it. (See the mypy documentation and avoid the Any
type.) Given that type checking is finding real bugs, we should be very hesitant to disable it for the sake of stylistic preferences.
There will always be some cases where appeasing the type checker rather than disabling it will mean writing code in a different way than you might have otherwise. No static type system can be powerful enough to capture all the ways a language as dynamic as Python can be used. I acknowledge that this is unfortunate. But I hope you’ll find as I have that the benefits of full type checking outweigh these inconveniences.
I think the same could be said for
or []
, which is also fragile based on the value ofob.__bool__
.
It’s conventional in Python, to the point of being enshrined in PEP 8, that testing a sequence as a boolean is an idiomatic check for emptiness. But let’s consider all the corner cases:
- A sequence that’s truthy but empty is fine since the result of
or []
is still empty. - A sequence that’s falsy but nonempty would be treated as empty—given the PEP 8 idiom, if you write such a sequence, you should have expected that some users will treat it that way—but this doesn’t break type safety.
- There’s no value that’s
None
but truthy, sinceNoneType
cannot be subclassed.
If that doesn’t satisfy you, here are some alternatives that type check:
def _top_level_inferred(dist: Distribution) -> Set[str]:
return {
f.parts[0] if len(f.parts) > 1 else f.with_suffix('').name
for f in ([] if dist.files is None else dist.files)
if f.suffix == ".py"
}
def _top_level_inferred(dist: Distribution) -> Set[str]:
if dist.files is None:
return set()
return {
f.parts[0] if len(f.parts) > 1 else f.with_suffix('').name
for f in dist.files
if f.suffix == ".py"
}
Perhaps the separate if
statement is actually the clearest?
Signed-off-by: Anders Kaseorg <[email protected]>
Signed-off-by: Anders Kaseorg <[email protected]>
Signed-off-by: Anders Kaseorg <[email protected]>
This functionality is needed by PackagePath, which previously called .open(). Signed-off-by: Anders Kaseorg <[email protected]>
ba1cb66
to
7c4e81e
Compare
|
||
class EntryPointBase(NamedTuple): |
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 effort is going to conflict with #339.
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.
No problem, we can update either PR to remove this base class if the other is merged first.
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.
|
||
class EntryPointBase(NamedTuple): | ||
name: str |
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 thought some of these attributes are allowed to be None. But maybe this indicates that they're not.
"""Load the entry point from its definition. If only a module | ||
is indicated by the value, return that module. Otherwise, | ||
return the named object. | ||
""" | ||
match = self.pattern.match(self.value) | ||
assert match is not 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.
Are these assertions relevant to type checking? I'm unsure these assertions add value. They introduce a new exception that might be raised (AssertionError) but only when optimization is turned off. It seems like it would be better to fail consistently irrespective of optimization flags even if that means the error is slightly less obvious. Let's omit these for now.
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.
Yes, pattern.match
returns Optional[Match[str]]
, so without this assert
, mypy (correctly) flags a type error on the next line:
importlib_metadata/__init__.py:197: error: Item "None" of "Optional[Match[str]]" has no attribute "group"
I assumed that nobody would be trying to specifically catch the AttributeError
that would have been raised at runtime by match.group
if match
was None
because the pattern didn’t match. But if we want to define a more specific exception for this case, we could do that.
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.
Agreed. I don't expect consumers to be relying on a specific exception. I'm less concerned with changing the exception than I am about the exception changing depending on the optimization flags. One of the features I like about Python is that you don't have to double-check every conditional at run-time. I like writing the minimal amount of code to achieve the primary purpose and hope that exceptions clearly indicate the proximate cause of exceptional conditions. The existing implementation does exactly what I'd want - it assumes the type is a match and fails otherwise. In this case, the assert adds noise to the implementation and runtime variability in the interface.
I don't like how mypy
is making us second-guess ourselves here. This issue occurs in other places in the code where the need for an assertion causes the need to introduce new scopes of execution (replacing an expression with multiple lines of execution adding names to the local namespace).
Probably the right thing to do here is for Python's regex object to grow a new method (e.g. ensure_match
or similar) that unconditionally returns a match or raises an exception if no match is found.
Lacking that functionality, perhaps this project should supply that functionality (provide a wrapper that ensures that interface). Such a wrapper could also raise an exception with extra context about what didn't match on what input.
Alternatively, this assertion could be couched in an if typechecking
block so it's clear that it's typing noise and not essential to the implementation.
assert match is not None | |
if typing.TYPE_CHECKING: | |
assert match is not None |
On further consideration, I don't like that approach because it still requires breaking out a simple expression into a block of code. I'd say that any solution should be implemented as a wrapper function that's usable in an expression.
assert match is not None | |
assert_not_none(match) |
assert match is not None | |
cast(match, MatchObject) |
|
||
def pop(self, *args, **kwargs): | ||
def pop(self, index: int = -1) -> _T: |
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.
It bothers me that these pass-through implementations require re-declaring the interface - that there's no way to inherit or infer the interface from the wrapped method. It also means that any variance in the implementation across Python versions will need to be kept in sync, adding technical debt and a maintenance burden. I've reported this issue with mypy (python/mypy#10149), but I'm not hopeful it will be addressed.
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.
One alternative would be to generate most of these wrapped methods dynamically?
def _wrap_deprecated_method(method_name: str) -> Any:
def wrapped(self: 'DeprecatedList[_T]', *args: Any, **kwargs: Any) -> Any:
self._warn()
return getattr(super(), method_name)(*args, **kwargs)
return wrapped
class DeprecatedList(List[_T]):
…
for method_name in [
'__setitem__',
'__delitem__',
'append',
'reverse',
'extend',
'pop',
'remove',
'__iadd__',
'insert',
'sort',
]:
locals()[method_name] = _wrap_deprecated_method(method_name)
Mypy won’t have any understanding that these methods are being overridden, but it doesn’t need to, because the signatures inherited from the superclass are fine.
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.
Yes. I like that, and it removes copy/paste code. One thing to consider/check is that the stacklevel is still correct in the warning, but I think it would be. I'd like to see this refactor adding prior to adding the annotations. I can do that.
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 tried implementing it as written, but when I did so, the super()
call failed:
269 >>> dl[0] = 1
UNEXPECTED EXCEPTION: RuntimeError('super(): __class__ cell not found')
Traceback (most recent call last):
File "/Library/Frameworks/Python.framework/Versions/3.10/lib/python3.10/doctest.py", line 1348, in __run
exec(compile(example.source, filename, "single",
File "<doctest importlib_metadata.DeprecatedList[2]>", line 1, in <module>
File "/Users/jaraco/code/public/importlib_metadata/importlib_metadata/__init__.py", line 257, in wrapped
return getattr(super(), method_name)(*args, **kwargs)
RuntimeError: super(): __class__ cell not found
/Users/jaraco/code/public/importlib_metadata/importlib_metadata/__init__.py:269: UnexpectedException
Moving the _wrap_deprecated_method
into the class namespace solved that issue.
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.
As I was working on an implementation, I ran into an emergent failure. It seems that in the past couple of hours, the tests have started failing on Python 3.10. That's a re-run of the test on main that previously passed, the same code as was released with the v4.8.1 tag. I'll need to track that down first.
time passes
Typing issue identified and fixed in 2448242.
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.
In 5875f39, I implement the alternative. Annoyingly, I had to apply type: ignore
to the _wrap_deprecated_method
because mypy did not like a function without a self
parameter:
293: error: Self argument missing for a non-static method (or an invalid type for self)
Maybe you'll have some ideas on how to improve the type declaration around that usage.
) | ||
return _adapters.Message(email.message_from_string(text)) | ||
|
||
@property | ||
def name(self): | ||
def name(self) -> str: |
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 suspect missing metadata values can be None. I'd have to double-check.
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.
PackageMetadata.__getitem__
is currently annotated as returning str
. Is the protocol wrong here?
importlib_metadata/importlib_metadata/_meta.py
Lines 15 to 16 in 0bc774f
def __getitem__(self, key: str) -> str: | |
... # pragma: no cover |
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.
Confirmed - None
is expected when the key doesn't exist.
>>> import importlib_metadata
>>> md = importlib_metadata.metadata('importlib_metadata')
>>> md['foo']
>>> md['foo'] is None
True
So probably the protocol is also wrong.
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.
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.
Not really, just me not getting enough sleep and forgetting this detail. This would have been caught if we had strict type hinting.
result = PackagePath(name) | ||
result.hash = FileHash(hash) if hash else None | ||
result.size = int(size_str) if size_str else None | ||
result.dist = self | ||
return result | ||
|
||
return file_lines and list(starmap(make_file, csv.reader(file_lines))) | ||
return ( |
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.
Does the typechecker not support A and B
?
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.
It does, but we have A: Optional[Iterable[str]]
and B: List[PackagePath]
; if A
is []: List[str]
, then A and B
is []: List[str]
rather than []: List[PackagePath]
, so it doesn’t satisfy the return type annotation.
(Why can’t empty lists be converted to different types? Because then you might append
a value of one type and observe it at the other type.)
And in general, A
might be some other falsy iterable with totally different behavior.
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.
Right, so here's a situation where the implementation is perfectly fine - it will always do what we want, but the type system is unable to statically verify that it's so. I was okay with the inelegance of "A and B" because it fit on one line. If this has to break out into multiple lines just to pass through a None value, then I'd probably want a different abstraction to keep this method concise.
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.
In 1e19fd2, I've refactored the implementation to retain the concise implementation and to tuck the None if x is None else
into a decorator from jaraco.functools
. Can you work with that?
|
||
|
||
# from jaraco.collections 3.3 | ||
class FreezableDefaultDict(collections.defaultdict): | ||
K = TypeVar('K') |
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 code no longer honors the comment above it. These annotations should be implemented in jaraco.collections first and then ported here.
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.
Proposed a simpler alternative in #346.
)(key) | ||
|
||
def freeze(self) -> None: | ||
if self.default_factory is not 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.
This change to the behavior probably deserves an issue report, regression test, and separate PR.
|
||
class Pair(NamedTuple): | ||
name: Optional[str] | ||
value: Any # Python 3.6 doesn't support generic NamedTuple |
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 don't understand the purpose of the comment.
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.
We use Pair
at two different value
types, Pair(name: str, value: str)
and Pair(name: Optional[str], value: Pair(name: str, value: str))
. I would have liked to make this a generic type argument:
from typing import NamedTuple, Generic, TypeVar, Optional
T = TypeVar("T")
class Pair(NamedTuple, Generic[T]):
name: Optional[str]
value: T
so we could express these as Pair[str]
and Pair[Pair[str]]
. However, a NamedTuple
cannot be Generic
at runtime in Python 3.6:
TypeError: metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases
This was fixed in Python 3.7.
Perhaps the most pragmatic solution is to use two different NamedTuple
classes?
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 news is I'm dropping support for Python 3.6 across a wide range of projects, so that should make porting this functionality to them easier.
|
||
# from jaraco.functools 3.3 | ||
def method_cache(method, cache_wrapper=None): | ||
def method_cache( |
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.
These changes should be first introduced in jaraco.functools.
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 started looking into porting this functionality to jaraco.functools in jaraco/jaraco.functools#19. Unfortunately, introduction of the types triggers failures in pytest-mypy.
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.
it wasn't until I tried porting these changes back to jaraco.functools that I remembered that this function wasn't a direct copy from jaraco.functools, but a modified version with _special_method_cache
removed (unused in this context), reminding me of the dangers of copy/pasting code. Fortunately, the tests were sufficient to catch the failure. Probably when bringing a type-annotated version of this function over here, we should be careful to stub _special_method_cache
rather than modifing the function.
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.
Thanks so much for putting in all this work. I really appreciate it. It's clear from #10 and #288, that many people will benefit from these changes. I'm a little dismayed at some of the effects adding typechecks imposes, but on the whole, it's looking good.
Please excuse my unfamiliarity with these thorough type annotations and feel free to educate me where necessary. If we can address the concerns I've raised, I believe this change is acceptable.
@@ -7,11 +7,11 @@ | |||
] | |||
|
|||
|
|||
def pytest_configure(): | |||
def pytest_configure() -> 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.
There seem to be an awful lot of these and they don't add a lot of value. Is there a mypy option that would allow this to be the default annotation, saving all the noise?
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.
No; for compatibility with untyped code, mypy needs to assume that a function without a type annotation is untyped, and treat its parameters and return values as Any
.
62c3715
to
cd4882f
Compare
Signed-off-by: Anders Kaseorg <[email protected]>
Signed-off-by: Anders Kaseorg <[email protected]>
Signed-off-by: Anders Kaseorg <[email protected]>
Signed-off-by: Anders Kaseorg <[email protected]>
always_iterable cannot be correctly annotated in Python’s type system. Signed-off-by: Anders Kaseorg <[email protected]>
There’s no using benefit to using functools.partial here since it’s only called once. Signed-off-by: Anders Kaseorg <[email protected]>
dist was only set to None temporarily during construction. This made the type annotation more complicated than necessary. Signed-off-by: Anders Kaseorg <[email protected]>
Signed-off-by: Anders Kaseorg <[email protected]>
Signed-off-by: Anders Kaseorg <[email protected]>
Signed-off-by: Anders Kaseorg <[email protected]>
@@ -975,14 +1150,13 @@ def entry_points(**params) -> Union[EntryPoints, SelectableGroups]: | |||
:return: EntryPoints or SelectableGroups for all installed packages. | |||
""" | |||
norm_name = operator.attrgetter('_normalized_name') | |||
unique = functools.partial(unique_everseen, key=norm_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.
I think I mentioned it elsewhere, but one reason I wrote it this way was because I was thinking of exposing unique
for other consumers to re-use (around other uses of distributions()
).
|
||
|
||
@overload | ||
def entry_points(*, module: str, **params: str) -> EntryPoints: |
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.
Although it's possible, I'd not expect callers to use value=
, module=
, or attr=
and I'd not want to declare it as supported unless we chose specifically to support it. I mainly expect users to use name=
and group=
.
That is, I was okay with the functionality being present but not advertised. Now the type system makes explicit which parameters might be selected and which not, causing the implementation to ripple through the type definition (and requiring changes in multiple places where previously they were simply passed through).
I guess since this goes away once the deprecated interface is removed, that's acceptable.
It sure would be nice if mypy has a feature request that would support this case (overload based on the presence of unnamed keyword arguments).
... # pragma: no cover | ||
|
||
def __truediv__(self) -> 'SimplePath': | ||
def __truediv__(self, other: Union[str, 'PathLike[str]']) -> 'SimplePath': |
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.
Path
uses key
as the parameter name, other
produces an error in Pyright:
def __truediv__(self, other: Union[str, 'PathLike[str]']) -> 'SimplePath': | |
def __truediv__(self, key: Union[str, 'PathLike[str]']) -> 'SimplePath': |
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.
(Arguably a bug in Pyright.)
@@ -1,32 +1,32 @@ | |||
from pytest_perf.deco import extras | |||
|
|||
|
|||
@extras('perf') | |||
def discovery_perf(): | |||
@extras('perf') # type: ignore[misc] |
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'd like to see if it's possible to ignore this file altogether. This file has a special purpose and isn't integrated with any of the codebase. Is it possible to simply exclude it from the checks to avoid the noise?
I want to thank you again for this pull request. I can see a lot of work went into it, but I also have some serious outstanding concerns about it. In particular, I'd like for the implementation to be primary and not influenced by the constraints of the type checkers. In other words, if the implementation is sound, it should be type-checkable. I'd be happy to consider subsequent contributions that are more selective and avoid the concerns I raised above. |
Add type annotations to every function, and correct some existing annotations, so that we pass mypy in strict mode. This makes the annotations more useful to downstream projects and improves our confidence in their correctness.