Skip to content
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

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

gabrieldemarmiesse
Copy link
Contributor

Maybe in the future we can even use plum's dispatch instead of if method is None:, we would be using plum inside plum!

@wesselb
Copy link
Member

wesselb commented Aug 17, 2023

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 get_overloads inside the dispatcher. Now the type signature is as follows:

    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.

@gabrieldemarmiesse
Copy link
Contributor Author

mmmhhh, you're right, i'll take another look once #93 is merged to see if I'm breaking some unit tests or not.

@wesselb
Copy link
Member

wesselb commented Aug 19, 2023

@gabrieldemarmiesse That makes sense! Let's do that.

@gabrieldemarmiesse gabrieldemarmiesse marked this pull request as draft August 20, 2023 16:25
@coveralls
Copy link

coveralls commented Aug 20, 2023

Pull Request Test Coverage Report for Build 5918488489

  • 7 of 9 (77.78%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 99.787%

Changes Missing Coverage Covered Lines Changed/Added Lines %
plum/dispatcher.py 6 8 75.0%
Totals Coverage Status
Change from base Build 5912676390: -0.2%
Covered Lines: 939
Relevant Lines: 941

💛 - Coveralls

@gabrieldemarmiesse
Copy link
Contributor Author

Before:
image

After:
image

It allows pycharm to undestand what the decorated function really is. And pycharm still give hints about the expected arguments

@gabrieldemarmiesse gabrieldemarmiesse marked this pull request as ready for review August 20, 2023 16:37
@gabrieldemarmiesse
Copy link
Contributor Author

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?

Copy link
Member

@wesselb wesselb left a 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. :)

plum/function.py Outdated Show resolved Hide resolved
plum/dispatcher.py Outdated Show resolved Hide resolved
plum/dispatcher.py Outdated Show resolved Hide resolved
plum/dispatcher.py Outdated Show resolved Hide resolved
plum/dispatcher.py Outdated Show resolved Hide resolved
@gabrieldemarmiesse
Copy link
Contributor Author

I tried to call methods with mypy but it doesn't seem to understand what is going on. I do a reveal_type and this is what I get:

tests/typechecked/test_overload.py:22: note: Revealed type is "Overload(def (x: builtins.int) -> builtins.int, def (x: builtins.str) -> builtins.str)"
tests/typechecked/test_overload.py:32: error: overloaded function has no attribute "methods"  [attr-defined]

So I can't really add a test for this.

@wesselb
Copy link
Member

wesselb commented Aug 22, 2023

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 pyright knows f.methods or not, but ignores the mypy error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants