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

Deprecate nested given #4283

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

tybug
Copy link
Member

@tybug tybug commented Mar 3, 2025

Closes #4167

@Zac-HD
Copy link
Member

Zac-HD commented Mar 3, 2025

Nice!

It occurs to me that some people are probably going to use .example()-inside-@given(), and if we can give a specific error message for that case it'd make more sense to them than talking about @given()-inside-@given().

@tybug
Copy link
Member Author

tybug commented Mar 3, 2025

I'm not sure how to rewrite our tests for this change without weakening them :( see e.g. test_minimise_array_shapes

@Zac-HD
Copy link
Member

Zac-HD commented Mar 4, 2025

...maybe we need to add a special "internal-use-only" flag to disable this for our own tests? It's janky, but I can see a reasonable argument for it.

@tybug
Copy link
Member Author

tybug commented Mar 4, 2025

If we can't rewrite our own tests without nested givens, maybe we shouldn't be deprecating them for others? Crosshair can pretty easily detect + throw when it encounters this, and I'm not sure that "quadratic behavior" is compelling for knowledgeable developers who are setting reasonable max_examples limits, as we do.

@jobh
Copy link
Contributor

jobh commented Mar 4, 2025

I agree, nested @given() (along with .example() tbh) is nice to be able to use, it's an intuitive idiom and not always worth the effort to figure out the best or most performant alternative.

Would it be acceptable to make it into a health check failure (so that it can be turned off after being warned about the drawbacks)?

@jobh
Copy link
Contributor

jobh commented Mar 4, 2025

I looked at the five reported in-the-wild usages. Briefly,

  1. Outer given is sampling from two elements (api variants) and should probably be replaced by parametrize (but relatively benign).
  2. Check that it is possible to find at least one valid suffix for any fixed prefix. Seems like something that is difficult to express without nesting?
  3. Indentation error (unintentional nesting).
  4. Outer given selects a type, inner given fuzzes over possible values of that type. They seem to be cognizant of the issues, disabling shrink for the outer.
  5. isn't nested (anymore?) afaics

2 and 4 are superficially valid uses IMO.

@Zac-HD
Copy link
Member

Zac-HD commented Mar 4, 2025

Would it be acceptable to make it into a health check failure (so that it can be turned off after being warned about the drawbacks)?

🤦‍♂️ yes this is obviously the solution, now that you mention it.

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.

Deprecate use of @given() inside @given()
3 participants