-
Notifications
You must be signed in to change notification settings - Fork 24
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
Make the dispatch function more explicit for type checkers with @overload #95
base: master
Are you sure you want to change the base?
Make the dispatch function more explicit for type checkers with @overload #95
Conversation
Hey @gabrieldemarmiesse! Do you have a specific test case that the linter should pick up, but doesn't? In the other PR, I've implemented your suggestion to add def __call__(self, method: Optional[T] = None, precedence: int = 0) -> T: with T = TypeVar("T", bound=Callable[..., Any]) I wonder what's right here, which is why I'm asking if you have a specific test case in mind. It would be nice to make that a unit test, if possible. |
mmmhhh, you're right, i'll take another look once #93 is merged to see if I'm breaking some unit tests or not. |
@gabrieldemarmiesse That makes sense! Let's do that. |
Pull Request Test Coverage Report for Build 5918488489
💛 - Coveralls |
I get a bit more output now with pyright but the tests are still passing. Do you think you could take a look and verify it's ok? |
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.
(Doing this from my phone, so I’m sorry in advance for any possible inaccuracies.)
Ah, this is nice!! I love seeing linter support improve with every PR.
Do you think that you’d be able to add the test from the screenshot as a unit test in test_overload? I think just accessing f.methods in the unit test should suffice, since it is checked by pyright and mypy.
If that checks out, then I’m happy to merge this right away. :)
I tried to call
So I can't really add a test for this. |
Co-authored-by: Wessel <[email protected]>
Co-authored-by: Wessel <[email protected]>
Co-authored-by: Wessel <[email protected]>
Co-authored-by: Wessel <[email protected]>
Co-authored-by: Wessel <[email protected]>
Hmm, well, if we can make pyright happy but can’t satisfy mypy, perhaps that’s already worthwhile testing for. How about something like the following? def test_methods():
# E: mypy(overloaded function has no attribute "methods")
# TODO: Make this work with `mypy`!
assert f.methods That should check whether |
Maybe in the future we can even use plum's dispatch instead of
if method is None:
, we would be using plum inside plum!