Skip to content

Allow hookspecs to declare default values for arguments - #732

Open
bluetech wants to merge 1 commit into
pytest-dev:mainfrom
bluetech:hookspec-defaults
Open

bluetech wants to merge 1 commit into
pytest-dev:mainfrom
bluetech:hookspec-defaults

Conversation

@bluetech

Copy link
Copy Markdown
Member

This is mainly a backward compatibility feature, to allow a hookspec to add new parameters to an existing hook without breaking existing callers.

The immediate motivation for this is coming from pytest-dev/pytest#15035, where pytest wants to add an argument to pytest_deselect hook but there are many existing external callers.

This feature has been discussed and implemented before:

although I haven't been able to figure out why it was rejected. To me it seems like an intuitive and natural feature, so I decided to put forward another attempt :)

The call overhead when default is not declared is a function call + 2 ifs, which seems OK to me. (Potentially can inline _apply_defaults but I still hold hope that cpython will one day do such trivial optimizations itself).

Note: there is a separate matter with defaults declared by hookimpls, which I think currently behave in an unintuitive way. There's an issue for it #442. I will try to tackle it next, as it has some interaction with this feature (see test_does_not_override_hookimpl_default test in this PR), but it's preexisting.

Fix #170.

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

my key nitpick form last time is that i only ever wanted to allow none as sentinel/default in order to prevent some types of drift

but i recon that this might have been too pedantic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Howto handle hook changes

2 participants