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

Error on excluded infinite endpoints #1860

Merged
merged 5 commits into from Mar 18, 2019
Merged

Conversation

@Zac-HD
Copy link
Member

Zac-HD commented Mar 9, 2019

Because there's nothing we can possibly generate in those cases. Closes #1859.

@Zac-HD Zac-HD added the bug label Mar 9, 2019
@Zac-HD Zac-HD force-pushed the Zac-HD:empty-intervals branch from db6b7e1 to 944f6e5 Mar 9, 2019
@DRMacIver

This comment has been minimized.

Copy link
Member

DRMacIver commented Mar 9, 2019

Without necessarily suggesting that this is a thing we should change in this PR, we might want to think about what the right thing to do in these cases is. We're currently a bit inconsistent - sometimes we raise InvalidArgument, sometimes we return an empty strategy. There are valid reasons to prefer one or the other in some cases, but I think maybe we need some guidelines and thinking about what those are and when we should prefer which.

@Zac-HD

This comment has been minimized.

Copy link
Member Author

Zac-HD commented Mar 9, 2019

Hmm, yeah - we should work out the principle and put it in the API style guide.

Personally I'd worry that more nothing() is likely to see a lot of user errors passing silently into weaker-than-expected tests, and thus favor InvalidArgument over the otherwise pleasant composability.

@DRMacIver

This comment has been minimized.

Copy link
Member

DRMacIver commented Mar 9, 2019

Personally I'd worry that more nothing() is likely to see a lot of user errors passing silently into weaker-than-expected tests, and thus favor InvalidArgument over the otherwise pleasant composability.

Yeah, I'm inclined to agree. Perhaps the solution is that we should provide a strategy for wrapping another strategy and turning InvalidArgument into nothing? We'd need to think a bit carefully about how to handle the transition of the current nothing options into raising InvalidArgument in a backwards compatible way but I think it's doable.

@Zac-HD

This comment has been minimized.

Copy link
Member Author

Zac-HD commented Mar 9, 2019

We'd need to think a bit carefully about how to handle the transition of the current nothing options into raising InvalidArgument in a backwards compatible way but I think it's doable.

For the transition, according to git grep the only public API that can return nothing() is sampled_from() - all five of the other uses are internal. So I think a garden-variety note_deprecation() would do, and then replace that with an error when we next bump the major version.

"One" is small enough n that I don't think a wrapper is worth it.

@Zac-HD Zac-HD force-pushed the Zac-HD:empty-intervals branch from 944f6e5 to fa88318 Mar 13, 2019
@Zac-HD Zac-HD force-pushed the Zac-HD:empty-intervals branch from 4ed58cd to d1bf1d6 Mar 14, 2019
@Zac-HD Zac-HD requested a review from DRMacIver Mar 18, 2019
Copy link
Member

DRMacIver left a comment

Comment on release note, otherwise LGTM and I don't need to rereview once that's fixed.


:func:`floats(min_value=0.0, max_value=-0.0) <hypothesis.strategies.floats>`
is now deprecated. While comparison operators would allow ``0.0 <= x <= -0.0``
for x as either signed zero, the interval is negative and thus disallowed.

This comment has been minimized.

Copy link
@DRMacIver

DRMacIver Mar 18, 2019

Member

I don't think "the interval is negative" makes sense as a phrase. What do you intend it to mean?

@Zac-HD Zac-HD force-pushed the Zac-HD:empty-intervals branch from 0d9bf39 to 3b71a7f Mar 18, 2019
@Zac-HD Zac-HD merged commit fc469a6 into HypothesisWorks:master Mar 18, 2019
4 checks passed
4 checks passed
HypothesisWorks.hypothesis #20190318.11 succeeded
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Zac-HD Zac-HD deleted the Zac-HD:empty-intervals branch Mar 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.