Skip to content

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

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

4 participants