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

Use signature so that we support positional-only arguments #3241

Merged
merged 7 commits into from Feb 28, 2022

Conversation

Zac-HD
Copy link
Member

@Zac-HD Zac-HD commented Feb 28, 2022

This patch addresses about two-thirds of #2706 - notably,

  • @st.composite strategies may now use positional-only arguments, which were added in Python 3.8, and
  • the first arguments to st.builds() and the Django extra's from_model() are now true positional-only arguments

This is basically just a nice cleanup to have; for API clarity I don't intend to support nontrivial signatures in @given()-wrapped tests. That's also some scary core code, so I'm leaving the relevant no-user-impact refactoring in there for later.

@Zac-HD Zac-HD added the legibility make errors helpful and Hypothesis grokable label Feb 28, 2022
@Zac-HD Zac-HD force-pushed the use-signature branch 4 times, most recently from e290bb1 to e8730ad Compare February 28, 2022 06:43
@honno honno self-requested a review February 28, 2022 21:01
Copy link
Member

@honno honno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to familiarise myself more with the reflection stuff so yeah, but LGTM! Excited for pos-only adoption.

@@ -22,8 +22,10 @@
from tokenize import detect_encoding
from types import ModuleType
from typing import TYPE_CHECKING, Callable
from unittest.mock import _patch as PatchType
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunate unittest doesn't expose this publicly, but I guess this has and will remain pretty stable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, our internals do (have to) depend on a lot of technically-private stuff. Best mitigation is extensive testing, including on alpha versions of new Python releases - if something breaks we should find out far enough in advance to fix it before the stable release.

@Zac-HD Zac-HD merged commit aa3564f into HypothesisWorks:master Feb 28, 2022
@Zac-HD Zac-HD deleted the use-signature branch February 28, 2022 21:42
@@ -90,7 +90,7 @@ def __init__(self, grammar, start, explicit):
# This is a total hack, but working around the changes is a nicer user
# experience than breaking for anyone who doesn't instantly update their
# installation of Lark alongside Hypothesis.
compile_args = getfullargspec(grammar.grammar.compile).args
compile_args = signature(grammar.grammar.compile).parameters
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think changing this to signature(grammar.grammar.compile, follow_wrapped=False).parameters will preserve parity with getfullargspec(grammar.grammar.compile).args / will address #3245

@@ -1463,29 +1467,32 @@ def composite(f: Callable[..., Ex]) -> Callable[..., SearchStrategy[Ex]]:
else:
special_method = None

argspec = getfullargspec(f)
sig = signature(f)
Copy link
Contributor

@rsokl rsokl Mar 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To preserve behavior of getfullargspec:

Suggested change
sig = signature(f)
sig = signature(f, follow_wrapped=False)

):
defaults[name] = value
if len(argspec.args) > 1 or argspec.defaults:
sig = signature(self.function)
Copy link
Contributor

@rsokl rsokl Mar 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
sig = signature(self.function)
sig = signature(self.function, follow_wrapped=False)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(there are additional instances below, but I'll hold off on pointing them all out in case I am simply wrong here)

@Zac-HD
Copy link
Member Author

Zac-HD commented Mar 2, 2022

I think we actually do want the new behaviour here, and it's on pytest-asyncio for using our internals to update.

@rsokl
Copy link
Contributor

rsokl commented Mar 2, 2022

Fair enough! This caused an error in MyGrad's test suite, but only because I was using @wraps in a totally inappropriate and bizarre way. So actually, this got me to clean up my test internals a bit!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
legibility make errors helpful and Hypothesis grokable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants