plum icon indicating copy to clipboard operation
plum copied to clipboard

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

Open gabrieldemarmiesse opened this issue 2 years ago • 8 comments

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

gabrieldemarmiesse avatar Aug 17 '23 11:08 gabrieldemarmiesse

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.

wesselb avatar Aug 17 '23 18:08 wesselb

mmmhhh, you're right, i'll take another look once https://github.com/beartype/plum/pull/93 is merged to see if I'm breaking some unit tests or not.

gabrieldemarmiesse avatar Aug 17 '23 18:08 gabrieldemarmiesse

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

wesselb avatar Aug 19 '23 16:08 wesselb

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%
<!-- Total: 7 9
Totals Coverage Status
Change from base Build 5912676390: -0.2%
Covered Lines: 939
Relevant Lines: 941

💛 - Coveralls

coveralls avatar Aug 20 '23 16:08 coveralls

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 avatar Aug 20 '23 16:08 gabrieldemarmiesse

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?

gabrieldemarmiesse avatar Aug 20 '23 16:08 gabrieldemarmiesse

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 avatar Aug 21 '23 17:08 gabrieldemarmiesse

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.

wesselb avatar Aug 22 '23 14:08 wesselb