Chore: Correct and verify type hints in manager.py#41
Conversation
| """ | ||
| try: | ||
| return func(*args) | ||
| func(*args, **{}) |
There was a problem hiding this comment.
None of the invocations of _call_safe do anything with the result, so I think this is a safe refactor/improvement
There was a problem hiding this comment.
In this case, could we specify the Callable as [PS, None]? Or - to keep the return, use -> T | None?
There was a problem hiding this comment.
Yep, that makes sense - I changed it to [PS, None]. If we need the result in the future, we can always change it to -> T | None, considering it's a private method.
| def test_lifecycle_listener(self, dummy_plugin_finder): | ||
| listener = MagicMock() | ||
| def test_lifecycle_listener(self, dummy_plugin_finder: DummyPluginFinder) -> None: | ||
| listener = Mock() |
There was a problem hiding this comment.
In the PluginManager I also made this change:
- if isinstance(listener, (list, set, tuple)):
+ if isinstance(listener, Iterable):Turns out that MagicMock is an instance of Iterable - so that suddenly changed the logic, and cause the test to fail. Using a basic Mock does not have this problem.
There was a problem hiding this comment.
Makes sense. I wonder if we should just narrow down the supported types on the interface instead - but I agree with not making any outward facing changes to the API in this PR, so this change is fine with me. We could have also just wrapped it in a list from the start, but I think it's fine like this!
dfangl
left a comment
dfangl
left a comment
There was a problem hiding this comment.
You captured a lot of typing issues, nicely done! LGTM!
| """ | ||
| try: | ||
| return func(*args) | ||
| func(*args, **{}) |
There was a problem hiding this comment.
In this case, could we specify the Callable as [PS, None]? Or - to keep the return, use -> T | None?
| def test_lifecycle_listener(self, dummy_plugin_finder): | ||
| listener = MagicMock() | ||
| def test_lifecycle_listener(self, dummy_plugin_finder: DummyPluginFinder) -> None: | ||
| listener = Mock() |
There was a problem hiding this comment.
Makes sense. I wonder if we should just narrow down the supported types on the interface instead - but I agree with not making any outward facing changes to the API in this PR, so this change is fine with me. We could have also just wrapped it in a list from the start, but I think it's fine like this!
bblommers
force-pushed
the
typing-manager
branch
from
3c5fc3b to
84924a5
Compare
2 participants
Motivation
The
PluginManageralready had decent type hints - this PR improves them a bit and adds support formypyto verify that they are all correct.Note that I added type hints to the test as well, just to verify the type hints of the
PluginManageritself are correct when invoking the class.Ideally I'd like to get to a point where we add types to the entire code base, just to make it a little easier to use downstream. When we get to that point, we can add the
py.typedfile as well.