Skip to content

Confusion in documentation: "Declaring New Hooks" #7706

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

Closed
mcow opened this issue Sep 2, 2020 · 6 comments · Fixed by #7782
Closed

Confusion in documentation: "Declaring New Hooks" #7706

mcow opened this issue Sep 2, 2020 · 6 comments · Fixed by #7782
Labels
good first issue easy issue that is friendly to new contributor type: docs documentation improvement, missing or needing clarification

Comments

@mcow
Copy link

mcow commented Sep 2, 2020

In this section there are several details in the examples that could be improved for less confusion:

  • The sample hook declaration file is hook.py but the import of it reads import hooks; the names do not match. Consider using a less generic name, such as sample_hook.py.
  • Related to that is this line in the documentation: "hooks are called through the hook object, available in the config object." Is "the hook object" always named hook (yes it is, but that's the confusing point) or is it named hook because the example module is hook.py?
  • Even tho it's not required (I guess, this is not done in the xdist example), for clarity please decorate the pytest_addhooks() implementations with hookimpl (with the AT sign, of course)? Documentation somewhere that explains when/whether this decoration is optional would also be appreciated.
  • "Hooks may be called both from fixtures or from other hooks." Please clarify that a hook may be invoked from a function or method that has access to the Config object; this is done in the xdist example, in fact.
@nicoddemus
Copy link
Member

All good points @mcow, thanks!

@nicoddemus nicoddemus added good first issue easy issue that is friendly to new contributor type: docs documentation improvement, missing or needing clarification labels Sep 3, 2020
@kamahmad
Copy link
Contributor

kamahmad commented Sep 12, 2020

@nicoddemus Can I please work on this issue?

@nicoddemus
Copy link
Member

@kamahmad sure, no need to ask. 😁 👍

@kamahmad
Copy link
Contributor

@nicoddemus this is the first time I'm contributing to open source so might need a little help. :)

For point 1 and 2, I have changed the doc - hooks.py is now sample_hook.py and import hooks is now import sample_hook.

For point 3, I have decorated pytest_my_hook and pytest_addhooks with hookimpl. Should I also add a line to say where it is required/optional? If yes, what should that line say?

@nicoddemus
Copy link
Member

Sorry for the delay, lots of things going around lately!

@nicoddemus this is the first time I'm contributing to open source so might need a little help. :)

Sure, welcome! Just so you now, it is perfectly fine to open an incomplete PR and ask questions over there. 😁 You can use GH's draft PRs to better communicate that.

For point 1 and 2, I have changed the doc - hooks.py is now sample_hook.py and import hooks is now import sample_hook.

Sounds good.

For point 3, I have decorated pytest_my_hook and pytest_addhooks with hookimpl. Should I also add a line to say where it is required/optional? If yes, what should that line say?

No need to point that out in that example; pytest doesn't require an explicit hookimpl decorator, we only use it when we need to configure hook-specific options (such as tryfirst). No need to mention that in that first example, which is more of an introduction.

Thanks!

@kamahmad
Copy link
Contributor

@nicoddemus - no worries :) thanks for answering my questions.

I've created a draft PR as you suggested. Can you please have a look and let me know if it's ok.

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue easy issue that is friendly to new contributor type: docs documentation improvement, missing or needing clarification
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants