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

Detailed error message on hook registration conflict #379

Merged
merged 4 commits into from
Jun 21, 2023

Conversation

vitek
Copy link
Contributor

@vitek vitek commented Mar 24, 2023

Hi! I've spent some time trying to nail down hook registration error:

INTERNALERROR>     pluginmanager.add_hookspecs(Hookspec)
INTERNALERROR>   File "contrib/python/pluggy/py3/pluggy/_manager.py", line 175, in add_hookspecs
INTERNALERROR>     hc.set_specification(module_or_class, spec_opts)
INTERNALERROR>   File "contrib/python/pluggy/py3/pluggy/_hooks.py", line 200, in set_specification
INTERNALERROR>     assert not self.has_spec()
INTERNALERROR> AssertionError

This PR raises RuntimeError with detailed message which should ease developer's life a bit:

E           RuntimeError: Hook 'conflict' is already registered within namespace <class 'test_hookcaller.test_hook_conflict.<locals>.Api1'>

src/pluggy/_hooks.py:341: RuntimeError

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.

Thank you for getting this started, the pr is well prepared.

Please have a look at my comments.

@@ -337,7 +337,11 @@ def set_specification(
specmodule_or_class: _Namespace,
spec_opts: "_HookSpecOpts",
) -> None:
assert not self.has_spec()
if self.spec is not None:
raise RuntimeError(
Copy link
Member

Choose a reason for hiding this comment

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

This ough to be a ValueError or a subclass of it

pass

pm.add_hookspecs(Api1)
with pytest.raises(RuntimeError):
Copy link
Member

Choose a reason for hiding this comment

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

Please add a check for the message, just to be safe

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.

Well done, would you like to compact history or squash?

@vitek
Copy link
Contributor Author

vitek commented Mar 24, 2023

I usually do squash on merge. Should I do it myself?

@RonnyPfannschmidt
Copy link
Member

We currently don't have squash merge enabled on the repo, but of you consent to squash I'll happily do it later today after enabling

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.

LGTM, would you like to add a CHANGELOG entry as well?

@vitek
Copy link
Contributor Author

vitek commented Mar 24, 2023

LGTM, would you like to add a CHANGELOG entry as well?

No thank you, it's minor fix. Still can add line if it's required.

Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

Thanks!

@bluetech bluetech merged commit 926084e into pytest-dev:main Jun 21, 2023
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.

None yet

4 participants