-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
DOC: verify your changes #29187
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
DOC: verify your changes #29187
Conversation
doc/devel/contribute.rst
Outdated
|
||
#. **After starting** check that your contribution meets the :ref:`pull request guidelines <pr-author-guidelines>` | ||
and :ref:`update the pull request <update-pull-request>` as needed. | ||
#. **Then** start working on the problem by following the guidance in |
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.
#. **Then** start working on the problem by following the guidance in | |
#. **Then** start working on the problem, on your feature branch, by following the guidance in |
Thinking how do we distinguish the different stages of the git process b/c we jumble everything together, but also realizing we can't use local b/c of codespaces and GitHub GUI. So thinking instead of ownership - your branch vs opening a PR on Matplotlib.
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.
IMHO, we don’t need „feature branch“ here. It‘s one level to detailed: It‘s described in the link, and may be confusing for newcomers that don’t have read the link/are not familiar with the process.
Maybe also avoid „problem“ and instead use one of
#. **Then** start working on the problem by following the guidance in | |
#. **Then** start working on the topic by following the guidance in |
#. **Then** start working on the problem by following the guidance in | |
#. **Then** start solving the issue by following the guidance in |
Thanks for opening the PR! |
doc/devel/development_workflow.rst
Outdated
* If the issue you are working on provided a code example, run that example | ||
against your branch and check that you now get the desired result. |
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.
Do we wanna add something like "if there are no tests, turn it into a test if small"? or is that too much?
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.
"Note that adapting the issue example is often a good way to create a new test"?
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.
I'd like the note about adapting the issue example added, but I think this is great/clearer than what was there either way.
PR summary
Closes #29181. The aim is to set a minimum bar that a proposed solution to an issue should actually solve the issue (and not create others). At the same time, we do not want to give the impression that everything has to be perfect so things like adding a test come under "address as much of the PR Guidelines list as you can".
Also modified the "Start a PR" section so it is clearer that looking through the checklist comes before opening the PR.
PR checklist