Skip to content

typing: Improve FixtureDefinition and FixtureDef #13036

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

typing: Improve FixtureDefinition and FixtureDef #13036

wants to merge 4 commits into from

Conversation

@nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Dec 7, 2024

  • Carry around parameters and return value in FixtureFunctionDefinition.
  • Add FixtureParams to FixtureDef.

Follow up to #12473.

@nicoddemus nicoddemus mentioned this pull request Dec 7, 2024
Closed
@nicoddemus nicoddemus added the skip news used on prs to opt out of the changelog requirement label Dec 7, 2024
@nicoddemus
Copy link
Member Author

nicoddemus commented Dec 7, 2024

cc @Glyphack

@nicoddemus
typing: Improve FixtureDefinition and FixtureDef
0303285 
* Carry around parameters and return value in `FixtureFunctionDefinition`.
* Add `FixtureParams` to `FixtureDef`.

Follow up to pytest-dev#12473.
@nicoddemus nicoddemus force-pushed the typing-fixture-definition branch from ec71fce to 0303285 Compare December 7, 2024 13:03
@nicoddemus nicoddemus requested review from RonnyPfannschmidt and bluetech and removed request for RonnyPfannschmidt December 7, 2024 13:04
@nicoddemus
Copy link
Member Author

nicoddemus commented Dec 7, 2024

Hmmm turns out pytest-bdd uses FixtureDef in a few places:

https://github.com/search?q=repo%3Apytest-dev%2Fpytest-bdd%20FixtureDef&type=code

The new Generic parameter breaks it, and there is no support for default values in Generic parameters yet...

@bluetech
Copy link
Member

bluetech commented Dec 7, 2024

Is the FixtureDef change needed for the FixtureFunctionDefinition change?

@nicoddemus
Copy link
Member Author

nicoddemus commented Dec 7, 2024

Is the FixtureDef change needed for the FixtureFunctionDefinition change?

I don't think so. Should I try to keep only the change to FixtureFunctionDefinition?

RonnyPfannschmidt
RonnyPfannschmidt reviewed Dec 7, 2024
View reviewed changes
Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

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

The paramspec types give me a slight headache as we basically resolve them internally and I wonder if there's a good way to make them work in passover

src/_pytest/python.py Outdated

# Used for storing pseudo fixturedefs for direct parametrization.
name2pseudofixturedef_key = StashKey[dict[str, FixtureDef[Any]]]()
name2pseudofixturedef_key = StashKey[dict[str, FixtureDef[Any, Any]]]()
Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt Dec 7, 2024

Choose a reason for hiding this comment

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

I'm starting to get the impression we need a type alias for those dicts as they land in too many places

Copy link
Member Author

@nicoddemus nicoddemus Dec 7, 2024

Choose a reason for hiding this comment

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

Done.

@bluetech
Copy link
Member

bluetech commented Dec 7, 2024

I don't think so. Should I try to keep only the change to FixtureFunctionDefinition?

FixtureDef is internal and we mostly use object or Any as the type parameter, so if it breaks plugins we might as well keep it as is.

@nicoddemus
Copy link
Member Author

nicoddemus commented Dec 7, 2024

FixtureDef is internal and we mostly use object or Any as the type parameter, so if it breaks plugins we might as well keep it as is.

Done.

@nicoddemus nicoddemus force-pushed the typing-fixture-definition branch from f002e4a to 889d9b8 Compare December 7, 2024 22:52
@nicoddemus nicoddemus force-pushed the typing-fixture-definition branch from 889d9b8 to 348068c Compare December 7, 2024 22:53
@nicoddemus
Copy link
Member Author

nicoddemus commented Dec 7, 2024

TBH I'm second guessing if this is useful at all. In the end we cannot do proper type checking because we need to keep all the FixtureDefinition instances in the same data structure, so it all ends up being Any anyway...

@bluetech
Copy link
Member

bluetech commented Dec 8, 2024

I agree for FixtureDef, but FixtureFunctionDefinition is not stored in any datastructure, it's a decorator which wraps fixture functions. I think we should try to preserve the typing of fixture functions, even if we don't allow calling them. I can imagine some e.g. mypy plugin introspecting it. Also IDEs, etc.

@nicoddemus
Copy link
Member Author

nicoddemus commented Feb 8, 2025

Unfortunately I won't have time to continue this, so I will be closing given is a "nice to have" feature.

If anybody wants to continue from this point please be my guest!

@nicoddemus nicoddemus closed this Feb 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RonnyPfannschmidt RonnyPfannschmidt RonnyPfannschmidt left review comments

@bluetech bluetech Awaiting requested review from bluetech

Assignees

No one assigned

Labels

skip news used on prs to opt out of the changelog requirement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nicoddemus @bluetech @RonnyPfannschmidt