-
Notifications
You must be signed in to change notification settings - Fork 4
Add better static typing to matchers #270
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
base: main
Are you sure you want to change the base?
Add better static typing to matchers #270
Conversation
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 for this contribution! For future reference, I think this sort of change should've started off with an issue to have a discussion rather than jumping right in with a PR. I also won't have time to properly review this for at least a week because I'm on vacation!
I'll try to keep an eye on this PR to ensure CI runs on any changes you push, but you likely won't hear much else from me for a bit. In the meantime, I've got some requests and suggestions if you're interested in taking this further:
- I'd like to be pretty conservative with changes to the matchers, because there's a high risk of inadvertent breaking changes. I'd recommend scoping this PR down to solely trying to add a strict typing option to
Captor
- Deprecation warnings are pretty disruptive, too. I'd like to avoid deprecating any APIs as part of this effort
- For now, Decoy supports Python versions all the way back to 3.7, so one must be careful to avoid using typing syntax and features that are too recent
Otherwise, keep an eye on CI!
I'm actively working on Decoy v3 which will have the ability to break some APIs, so I appreciate that this PR can provide some inspiration to that effort, even if some changes might not be able to land right away. Feel free to leave any thoughts or ideas in the discussion.
For what it's worth, I'm considering dropping matchers from Decoy in the v3 release in favor of hamcrest. I know their matchers offer some form of strict typing, and they may already work with Decoy, so that might be something interesting for you to explore if the built-in matchers don't offer typing to your liking.
I'm also not sure if captors will be necessary in Decoy v3. It may prove more straightforward (and more strictly typed) to provide an API that returns the list of all calls to a mock
@@ -41,8 +41,8 @@ poetry run poe coverage | |||
|
|||
In an exciting twist, since version 1.6.0, Decoy's tests rely on Decoy itself to test (and more importantly, design) the relationships between Decoy's internal APIs. This means: | |||
|
|||
- Decoy's unit test suite serves as an end-to-end test of Decoy by virtue of existing (wow, very meta, actually kind of cool). | |||
- Changes that break a small part of Decoy may result in a large number of test failures, because if Decoy breaks it can't be used to test itself. | |||
- Decoy's unit test suite serves as an end-to-end test of Decoy by virtue of existing (wow, very meta, actually kind of cool). |
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.
todo: revert any unrelated formatting changes from the diff to make sure there's no unnecessary noise
@@ -61,7 +61,7 @@ poetry run poe format | |||
Decoy's documentation is built with [mkdocs][], which you can use to preview the documentation site locally. | |||
|
|||
```bash | |||
poetry run docs | |||
poetry run poe docs |
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.
praise: thanks for this fix!
from re import compile as compile_re, Pattern | ||
from typing import cast, TypeVar, Generic, Any, override, overload | ||
from collections.abc import Iterable, Mapping | ||
from warnings import deprecated |
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.
issue: deprecated
was only added to the warnings
module in Python 3.13, and I would like Decoy v2 to support all the way back to 3.7. Is this import required?
All things being equal, I would like to avoid deprecating anything
] | ||
|
||
|
||
MatchT = TypeVar("MatchT", default=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.
issue: the default
argument to TypeVar is a relatively recent addition to the language. Will this work on older versions of Python? Does this require adding typing_extensions
as a dependency?
Alternatively, can we be creative with our API design to avoid needing default types in generics at all?
|
||
|
||
class _IsA: | ||
_match_type: type | ||
_attributes: Optional[Mapping[str, Any]] | ||
_match_type: type[object] |
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.
issue: is type
generic in older versions of Python? Should this use typing.Type
instead?
@overload | ||
def Captor() -> Any: ... | ||
@overload | ||
def Captor(match_type: type[MatchT]) -> MatchT: ... |
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.
thought: I wonder if this overload provides a way to add the idea of a typed captor in a less disruptive way, e.g.:
@overload
def Captor(match_type: Type[MatchT]) -> TypedCaptor[MatchT]: ...
...
class TypedCaptor(NamedTuple, Generic[MatchT]):
captor: MatchT
values: CaptorValues[MatchT]
class CaptorValues(Protocol, Generic[MatchT]):
@property
def value(self) -> MatchT: ...
@property
def values(self) -> List[MatchT]: ...
Usage:
name_captor, name_values = Captor(str)
decoy.verify(greet(name_captor))
greet("Alice")
assert name_values.value == "Alice"
@@ -108,25 +108,63 @@ | |||
decoy.when(fake_any("hello")).then_enter_with(42) | |||
|
|||
out: | | |||
main:10: error: Invalid self argument "Stub\[int]" to attribute function "then_enter_with" with type "Callable\[\[Stub\[.*ContextManager\[ContextValueT]], ContextValueT], None]" \[misc] | |||
main:10: error: Invalid self argument "Stub\[int]" to attribute function "then_enter_with" with type "Callable\[\[Stub\[.*ContextManager\[ContextValueT, bool | None]], ContextValueT], None]" \[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.
issue: this test is known (to me) to fail on Python 3.13. Until I fix it, I recommend you use Python 3.12 to develop on Decoy. Your change here causes the tests to fail on Python <= 3.12
] | ||
|
||
for compare in comparisons: | ||
if isinstance(compare, int): |
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.
issue: I have a stance that if
conditions should be disallowed inside tests. Instead of an if
inside one test function, two (or more) test functions should be used
That being said, I can't really figure out why this if
is here. What is it doing?
This is a pretty verbose way of writing a test, so in general, you may want to approach using `matchers.Captor` as a form of potential code smell / test pain. There are often better ways to structure your code for these sorts of interactions that don't involve private functions. | ||
!!! tip | ||
|
||
If you want to only capture values of a specific type, or you would like to have stricter type checking in your tests, consider passing a type to [decoy.matchers.argument_captor][] (e.g. `argument_captor(match_type=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.
suggestion: this language implies that the value will be type-checked at runtime, which is not true (nor do I think it should be true!). Should this be phrased to make it more clear that this argument merely casts the value type?
I also think the admonition is a little unnecessary. I think this can be regular text
@@ -1,6 +1,6 @@ | |||
[tool.poetry] | |||
name = "decoy" | |||
version = "2.2.0" | |||
version = "2.3.0" |
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.
todo: please revert this change - version bumps are handled automatically by the release system
""" | ||
|
||
@abstractmethod | ||
def capture(self) -> CapturedT: |
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.
praise: I like this idea of a generic class with a type-cast property to pass to the stub configuration. I think it might be hard to land such a change in Decoy v2, but I think it could be a cool API to implement for all matchers in Decoy v3, if needed.
That way, there could be a public, generic Matcher
class, and we could drop a lot of the complexity/silliness around the existing marchers API
Summary
Improve static typing support for argument matchers.
Motivation and context
One of the selling points of Decoy, at least for me, is its well-typed interface, which improves the developer experience. However, the typing of argument matchers falls a bit short in this regard. The previous implementation relied heavily on using
Any
as a return type, but it can be improved with the use of Generics.Description of changes
Captor
matcher to provide a typed interface for getting the captured values, instead of having to get them from an object of typeAny
. By separating the object used to get the captured values and the object passed as a matcher, we get a typed interface with better support for for auto-completions in the IDE. I've also added an optional parameter to specify the type to match.Anything
orHasAttributes
), I use a single generic type variable as the return type.For pyright (which supports bidirectional inference), this means that the matcher infers the target type based on the context. See an example here. This approach is used by Mockito in Java.
For mypy, which is not capable of this type of inference (see an example here), the generic type is assigned its default value, which is
Any
for backwards compatibility.This change helps to remove errors and warnings of strict type checkers that report any usage of
Any
, like basedpyright.Testing
I have added both unit tests and type tests to verify the new expected behavior. I have also fixed a type test that was failing because of a syntax error.
Documentation
I have updated the documentation with the new functionality. I have also fixed a couple of errors that I found.