Skip to content

support PEP484 function argument annotation #65

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

Merged
merged 7 commits into from
Nov 13, 2017

Conversation

d2207197
Copy link
Contributor

No description provided.

@mrocklin
Copy link
Owner

Thanks for the work here @d2207197

This would need to pass existing tests (currently you're failing due to Python 2 failures) and also add new tests verifying that this works in all reasonable cases.

@d2207197 d2207197 closed this Sep 22, 2017
@d2207197 d2207197 reopened this Sep 22, 2017
@d2207197
Copy link
Contributor Author

  • passed all existing tests
  • added test for function annotation in tests/test_dispatcher_3only.py

@mrocklin
Copy link
Owner

I'm glad to see this. I hope you don't mind but I added a commit with a couple of tests here. It looks like this doesn't yet work as expected with methods? Any thoughts on how best to address this?

@d2207197
Copy link
Contributor Author

@mrocklin
Thanks for adding those tests. I've updated, and now it works with methods.

@mrocklin
Copy link
Owner

This looks good to me. CC'ing @oubiwann and @jlowin who have attempted this before to see if they have time to review.

@jlowin
Copy link

jlowin commented Oct 1, 2017

Wow, 2.5 years since #4 -- I think that was back when we were all trying to figure out the "right" way to handle annotations! I think these days we know that method in this PR IS the right way (inspect.signature and parameter.annotation).

One suggestion is to add a test almost identical to test_overlaps() but where the dispatch signature and the annotation signature disagree. The expected behavior is that dispatch is checked first and annotations are only checked if dispatch has no signature provided. Adding a test for that would ensure that the behavior doesn't get changed inadvertently in the future (for example, one day the annotations might be checked first and the dispatch signature would be used to override them).

@d2207197
Copy link
Contributor Author

d2207197 commented Oct 2, 2017

Thanks for the suggestion. I added the test case in 96d092b

@d2207197
Copy link
Contributor Author

d2207197 commented Oct 30, 2017

Is this PR ready to be merged in?
@mrocklin

@mrocklin
Copy link
Owner

I apologize for letting this linger. My inbox has become increasingly less organized/managed recently.

cc'ing @llllllllll in case he wants to take a look

@d2207197
Copy link
Contributor Author

Hi @mrocklin
Is there anything else I can do for this PR?

@mrocklin
Copy link
Owner

Sure. Looks good. I apologize again for my poor response on this.

Thank you so much for implementing this. I suspect that many people will appreciate it.

@mrocklin mrocklin merged commit 4dd36b1 into mrocklin:master Nov 13, 2017
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