Skip to content

Make the dispatch function more explicit for type checkers with @overload#95

Open
gabrieldemarmiesse wants to merge 11 commits intobeartype:masterfrom
gabrieldemarmiesse:add_last_type_hints
Open

Make the dispatch function more explicit for type checkers with @overload#95
gabrieldemarmiesse wants to merge 11 commits intobeartype:masterfrom
gabrieldemarmiesse:add_last_type_hints

Conversation

@gabrieldemarmiesse
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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. :)

Comment thread plum/function.py Outdated
Comment thread plum/dispatcher.py Outdated
Comment thread plum/dispatcher.py Outdated
Comment thread plum/dispatcher.py Outdated
Comment thread plum/dispatcher.py Outdated
@gabrieldemarmiesse
Copy link
Copy Markdown
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.

gabrieldemarmiesse and others added 6 commits August 21, 2023 19:12
Co-authored-by: Wessel <wessel.p.bruinsma@gmail.com>
Co-authored-by: Wessel <wessel.p.bruinsma@gmail.com>
Co-authored-by: Wessel <wessel.p.bruinsma@gmail.com>
Co-authored-by: Wessel <wessel.p.bruinsma@gmail.com>
Co-authored-by: Wessel <wessel.p.bruinsma@gmail.com>
@wesselb
Copy link
Copy Markdown
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