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

Remove misleading assume example. #4061

Merged
merged 1 commit into from
Sep 29, 2024

Conversation

abeakkas
Copy link
Contributor

@abeakkas abeakkas commented Aug 1, 2024

Given snippet doesn't find any satisfying examples anymore. Not sure if it can be fixed through tweaking some settings but it is misleading in its current state. I don't know what changed since the original documentation was added, maybe @DRMacIver can comment.

See the relevant PR:
#2600
which was mistakenly reverted in:
#3238

The given snippet doesn't find satisfying examples anymore. Not sure if it can be fixed through some settings but it is misleading in its current state.
@DRMacIver
Copy link
Member

Huh it's hard to see how this example would ever have passed in modern Hypothesis. I wonder how old it is - I think some of the early versions had features that might have allowed it to pass, but I think it's probably been broken since 3.0 came out. Thanks for the catch!

Copy link
Member

@DRMacIver DRMacIver left a comment

Choose a reason for hiding this comment

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

I think this section is a bit confusing with this example just straightforwardly deleted, and the example should be retained but with the actual error message produced and an explanation that this demonstrates the limitations of assume, suggesting rewriting it to use lists(integers(min_value=0), min_size=11) instead of the assume.

Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

I'm happy to merge this as-is; it reads well enough to be an improvement on the status quo IMO and it's not like we have a surplus of documentation contributors 😅

Thanks for the patch, @abeakkas!

@Zac-HD Zac-HD merged commit 75650b9 into HypothesisWorks:master Sep 29, 2024
58 checks passed
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.

None yet

3 participants