-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
docs: Create links for objects to show the api #2473
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks for this!
Just two things:
- I think this warrants a note in the CHANGELOG. 👍
- The change in
pluggy.py
should be applied upstream.
Other than that it is ready for merging, I've tested the changes locally and they work as expected.
@@ -540,7 +540,7 @@ def add_hookcall_monitoring(self, before, after): | |||
of HookImpl instances and the keyword arguments for the hook call. | |||
|
|||
``after(outcome, hook_name, hook_impls, kwargs)`` receives the | |||
same arguments as ``before`` but also a :py:class:`_CallOutcome`` object | |||
same arguments as ``before`` but also a :py:class:`_CallOutcome <_pytest.vendored_packages.pluggy._CallOutcome>` object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change should be applied in the upstream repository at pytest-dev/pluggy instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the real problem is, that we change the absolute path, there - the whole thing is incorrect unless we stop vendoring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"the whole thing" you mean the entire PR? I don't think so, only the change in pluggy
needs to be dropped, the other links work fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicoddemus what i mean is that the pluggy details we include in the docs go wrong because we change the absolute name of everything inside pluggy due to vendoring (so the change to pluggy is correct for the context of making the pytest docs correct, but it breaks vendoring
Thanks again @ApaDoctor! |
Fixes: #2331
head-fork: ApaDoctor/pytest
compare: docs-fixes
base-fork: pytest-dev/pytest
base: master # if it's a bugfix