Improve support for @st.composite with @classmethod or @staticmethod
#2634
Conversation
Zac-HD
force-pushed
the
composite-methods
branch
2 times, most recently
from
284552c
to
115f360
Compare
| '8z]EIFA06^l`i^': 'LFE{Q', | ||
| '8z]EIFA06^li^': 'LFE{Q', |
There was a problem hiding this comment.
This is just because the backtick broke syntax highlighting support in VS code, and I wanted to delay new contributors' first encounter with the Curse of Hypothesis.
|
Hey @HypothesisWorks/hypothesis-python-contributors, anyone want to review a PR? It's short and simple, promise 😇 |
hypothesis-python/docs/data.rst
Outdated
| @@ -302,6 +302,10 @@ lead to serious performance problems. For example: | |||
| result.add(draw(elements.filter(lambda x: x not in result))) | |||
| return result | |||
|
|
|||
| :func:`@composite <hypothesis.strategies.composite>` *can* be used on methods, | |||
| classmethods, and staticmethods, but note that the ``draw`` argument must come | |||
| before ``self`` or ``cls``. See :issue:`2578` for more details. | |||
There was a problem hiding this comment.
This will break several linters. At least pylint and wemake-python-styleguide.
And mypy too.
😢
There was a problem hiding this comment.
I think my response is "don't use non-static methods as composite strategies then"; personally I think that using a function is cleaner anyway.
The basic problem is that there's no way for us to distinguish between functions and methods - because at runtime methods are functions - if @composite is applied alone or before @classmethod. We are therefore left with a trilemma between this consistent-but-unlintable style, using unsound heuristics like guessing based on the argument name, or having the behaviour vary based on the order of decorators. Deprecating the use of @composite on methods is also out, because as above there's no way to tell at runtime.
Overall, I prefer this PR as an improvement on the status quo ("draw must come first, and @composite must be the inner-most decorator") even though I wouldn't actually use it for anything but staticmethods... and in that case why not use a function?
There was a problem hiding this comment.
@sobolevn - if you're happy with that explanation, you can approve and merge 😉
There was a problem hiding this comment.
Do we need to mention it in the docs, what do you think? 🤔
There was a problem hiding this comment.
I've clarified and expanded the docs, though without discussing the specific implementation constraints beyond linking to this PR.
I've also tweaked the test for #2646 as you suggested after merge 🙂
Zac-HD
force-pushed
the
composite-methods
branch
from
115f360
to
a84f580
Compare
Zac-HD
force-pushed
the
composite-methods
branch
from
a84f580
to
8c1fcbe
Compare
|
Thanks 🥰 |
Closes #2578.