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

Add more parameters to overridable methods for parameterized ID creation #12628

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

NicoloBorghi
Copy link

Suggesting new feature for parameterized ID name creation. In this way, all overridable options get the same set of arguments as the main _idval method.

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Jul 18, 2024
pre-commit-ci bot and others added 17 commits July 18, 2024 10:46
@NicoloBorghi
Copy link
Author

I have the feeling that unit tests are failing because the changes I made are being tested with pytest themselves, which in turn doesn't contain the changes.

I'm very open to suggestions.

Cheers

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.

I like the idea

We need to ensure backwards compatibility and I'd like to bike shed about argument names a little

@@ -953,7 +953,7 @@ def _idval_from_function(self, val: object, argname: str, idx: int) -> str | Non
if self.idfn is None:
return None
try:
id = self.idfn(val)
id = self.idfn(val, argname, idx)
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change,the old signature needs to be supported as well

I recommend using a callable protocol as starting points

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

We also need to update the docs, for example: https://docs.pytest.org/en/stable/example/parametrize.html

Besides this, as @RonnyPfannschmidt commented, this feature needs to be backward compatible, but I'm not sure how we can do that easily.

@NicoloBorghi
Copy link
Author

@nicoddemus

I apologize for putting up such rough code (mostly due to lack of experience). If you don't mind, we could find a way. Otherwise, feel free to decline and close this PR.

Having said this, would a very crude try/except block to catch a TypeError be a potential solution?

Thank you for your time.

Cheers

@nicoddemus
Copy link
Member

nicoddemus commented Jul 19, 2024

I apologize for putting up such rough code (mostly due to lack of experience). If you don't mind, we could find a way. Otherwise, feel free to decline and close this PR.

No problem at all, don't worry. This is good even if to at least start a conversation. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided (automation) changelog entry is part of PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants