Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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

Merged
merged 1 commit into from
Nov 27, 2024
Merged

DOC: verify your changes #29187

merged 1 commit into from
Nov 27, 2024

Conversation

rcomer
Copy link
Member

@rcomer rcomer commented Nov 25, 2024

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

doc/devel/contribute.rst Outdated Show resolved Hide resolved

#. **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
Copy link
Member

@story645 story645 Nov 25, 2024

Choose a reason for hiding this comment

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

Suggested change
#. **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.

Copy link
Member

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

Suggested change
#. **Then** start working on the problem by following the guidance in
#. **Then** start working on the topic by following the guidance in
Suggested change
#. **Then** start working on the problem by following the guidance in
#. **Then** start solving the issue by following the guidance in

doc/devel/contribute.rst Outdated Show resolved Hide resolved
@story645
Copy link
Member

story645 commented Nov 25, 2024

Thanks for opening the PR!

Comment on lines 154 to 155
* 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.
Copy link
Member

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?

Copy link
Member Author

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"?

doc/devel/development_workflow.rst Outdated Show resolved Hide resolved
Copy link
Member

@story645 story645 left a 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.

@timhoffm timhoffm merged commit 4d547e0 into matplotlib:main Nov 27, 2024
22 checks passed
@rcomer rcomer deleted the dev-verify branch November 27, 2024 17:22
@rcomer rcomer added this to the v3.11.0 milestone Nov 27, 2024
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.

[Doc]: locally testing changes
3 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.