DOCS: include how to reproduce CI in PR template

[brunobeltran]

PR Summary

Implement the suggestions to the PR checklist described in (https://discourse.matplotlib.org/t/how-to-engage-more-contributors-to-matplotlib/21059/11) and discussed in the last call.

Basically, gives users explicit instructions for how to "check" that each of the checkboxes is actually ready to check.

PR Checklist

• Has Pytest style unit tests
• Code is Flake 8 compliant
• New features are documented, with examples if plot related
• Documentation is sphinx and numpydoc compliant
• Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
• Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

[timhoffm]

General structure

I find this list rather intimidating (this is not a comment on your PR but on the list in general). I'm always put off if I have to read through lengthy PR instructions.

I propose to create subsections for different aspects of PR like this:

---

General:

• Code is Flake 8 compliant (run flake8 on changed files to check)
• ...

If new feature:

• New features are documented, with examples if plot related
• For major features add an entry to doc/users/next_whats_new/ (follow instructions in README.rst there)

If API changed:

• Documented in doc/api/api_changes_[VERSION] if API changed in a
  backward-incompatible way (follow instructions in README.rst there)

---

That way a user with simple bug fix or documentation fix doesn't have to read the new feature and API changed sections.

[timhoffm]

I think we can lift the restriction on pydocstyle<4. This was introduced in #14710 because of https://gitlab.com/pycqa/flake8-docstrings/issues/36 but is likely not neccessary anymore.

[brunobeltran]

That bug was fixed, but the latest versions (I think pydocstyle>5.0.1) now crash on figure.py, as per PyCQA/pydocstyle#437. 4.0 also crashes and I didn't feel like chasing versions around so I left it at the one that I knew already worked...

[brunobeltran]

I agree that the list is pretty imposing.

I agree that splitting it up into sections would do a lot to help ameliorate number of brain cells required to figure out which of the check boxes apply to a PR. We could even include a comment that lets people know they can delete sections that don't apply to them.

I think the main thing that would make it less imposing would actually be changing the title. PR Checklist sounds like something that someone would be upset at me for messing up. Can't think of a good replacement right now but maybe something along the lines of "Useful PR Checks".

[brunobeltran]

For easy reading of the "rendered" version, here's my proposed PR template, after including @timhoffm's suggestions:

PR Summary

Summary would go here. Usually, only one or two of the check-box sections below would apply, and the rest could be omitted.

PR Checklists

For new code:

• Has Pytest style unit tests (and pytest lib/matplotlib/tests passes)
• Code is Flake 8 compliant (run flake8 on changed files to check)
• New code is documented, with examples if plot related

For new documentation:

• Documentation is sphinx and numpydoc compliant (the docs should build without error)
• New documentation conforms to matplotlib style conventions. (If you have flake8-docstrings and pydocstyle<4 installed, run flake8 --docstring-convention=all on changed files to check).

For new features:

• Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)

For API changes:

• Documented in doc/api/api_changes_[VERSION] if API changed in a backward-incompatible way (follow instructions in README.rst there)

[story645]

Thanks for putting this in & I wrote most of this language so I'm mostly critiquing my earlier work 🙃

[story645]

Doesn't need the new (for consistency w/ above - if it's in the PR it should conform to the conventions)

[brunobeltran]

I guess I worry that users will try to submit a simple PR and think they have to clean up warnings from an entire file, instead of just making sure that the docs they add/touch follow the conventions.

[brunobeltran]

Although I guess it could be argued that if it fails flake8 --docstring-convention=all with our .flake8 file, it should be fixed, since we already explicitly ignore all "known" errors.

[story645]

Kind think maybe take out the descriptive pieces up top and replace w/ another checklist (folks seem to pay more attention to lists than to prose)
Pull Request
[] PR has descriptive name
[]PR has minimum of 1-2 sentence descriptive summary
[] optional:PR cross-links related issues

[brunobeltran]

So I included a "Meta" section. To be honest, I think it's a little clearer just as prose, especially since it makes the already imposing list of checklists even longer....but if we have better luck with the checkboxes I guess you gotta do what you know works....

[timhoffm]

What about making the "Meta" section bullet points? This is more like a reminder. I don't want to tick numerous boxes when making a PR. These things are easy to apply and there is no reason not to immediately do it (in contrast to e.g. writing good tests, where sometimes it's reasonable to open a PR without.)

Same would maybe apply to "New Documentation" section?

[brunobeltran]

I agree that I don't want to be checking off boxes for every PR when it's just things like "did you fill in the summary".

If I do make them just bullet points, then I see no need for it to ever be rendered (reviewer knows about these, don't need the visual noise), so I've changed it to be commented out by default.

Pasted latest version below.

[brunobeltran]

No description provided.

[brunobeltran]

PR Summary

PR Checklists

New code:

• Has Pytest style unit tests (and pytest lib/matplotlib/tests passes)
• Code is Flake 8 compliant (run flake8 on changed files to check)
• Code is documented, with examples if plot related

New documentation:

• Documentation is sphinx and numpydoc compliant (the docs should build without error)
• Documentation conforms to matplotlib style conventions. (If you have flake8-docstrings and pydocstyle<4 installed, run flake8 --docstring-convention=all on changed files to check).

New features:

• Have an entry in doc/users/next_whats_new/ (follow instructions in doc/users/next_whats_new/README.rst)

API changes:

• Documented in doc/api/api_changes_[NEXT_VERSION] if API changed in a backward-incompatible way (follow instructions in doc/api/api_changes_[NEXT_VERSION]/README.rst)

[timhoffm]

Not sure how this long line will look. Is it better to hard-wrap after a fixed number of characters? (Also for other lines)

[brunobeltran]

Yeah I think it makes sense to hard-wrap the comment lines. I played around with Github's current PR screen in dev mode and it has weird behavior on resizing but if we wrap at 79 characters we should be safe from having weird wrapping artifacts.

The checkbox lines interact funny with screen resizing because they're full of big "words" (e.g. doc/api/api_changes_[NEXT_VERSION]/README.rst), so I'd vote we leave them as single lines and let the Github text box wrap them for us "naturally".

If Github ever loses its mind and adds a horizontal slider to these boxes then we can come back and wrap those lines in the Markdown.

[story645]

Added some copyedits directly, but think since this is a bullet list now anyway:

• PR title summarizes the changes. (Example: "Raise ValueError on non-numeric input set_xlim" )
  • Please don't use non-descriptive titles such as "Addresses issue support 'markevery' in prop_cycle #8576"
• PR has at least a 1-2 sentence descriptive summary of how the PR implements the changes.

[story645]

I'd pull this to the top of the template since it's the first thing they should do.

[brunobeltran]

Thanks for the patience and great iterative feedback guys, I think we're getting close!

The organizational shift to have the section previously called "meta" at the top looks like it works really well, since it's now basically the first thing the user reads and avoids redundant language that way.

Decided to make all checkboxes sentence fragments (previously there was mis-mash of full sentences and fragments). Happy to instead go to all full sentences if this way looks silly to you guys, but figured they should all be the same, and this way is much shorter.

[brunobeltran]

PR Summary

PR Checklists

New code:

• has Pytest style unit tests (and pytest lib/matplotlib/tests passes)
• is Flake 8 compliant (run flake8 on changed files to check)
• is documented, with examples if plot related

New documentation:

• is sphinx and numpydoc compliant (the docs should build without error)
• conforms to matplotlib style conventions (if you have flake8-docstrings and pydocstyle<4 installed, run flake8 --docstring-convention=all on changed files to check).

New features:

• have an entry in doc/users/next_whats_new/ (follow instructions in doc/users/next_whats_new/README.rst)

API changes:

• are documented in doc/api/api_changes_[NEXT_VERSION] if API changed in a backward-incompatible way (follow instructions in doc/api/api_changes_[NEXT_VERSION]/README.rst)

[QuLogic]

Small typos and corrections.

[timhoffm]

I'm torn with the first and last section. While they provide useful and comforting information, this makes the template rather long, which I personally find discouraging.

The alternative would be to drastically shorten and just link
https://matplotlib.org/devdocs/devel/coding_guide.html#summary-for-pr-authors.

[brunobeltran]

@timhoffm, I actually agree that ideally this template would contain just the checklist (with no prose at the end). The actual prompt could be short and just read

<!-- 
Thank you for your PR! Please fill in this section with at least 1-2 sentences summarizing the purpose and implementation of your PR. 

Please take a moment to make sure your PR follows our [contributor guidelines]() before proceeding.
-->

But in order for that to work, IMO, we would need to be able to link a single page of the dev docs, and effectively have the checklist there. Currently, this is split over two places:

• https://matplotlib.org/devdocs/devel/coding_guide.html#summary-for-pr-authors
• https://matplotlib.org/devdocs/devel/contributing.html

I think centralizing these instructions is enough work for a separate PR, and we have made two very concrete improvements in this new version:

1. The sections make it much easier to understand what parts of checklist that don't apply and delete unnecessary ones.
2. The instructions for how to check that CI will pass being here is almost more important than the actual description, IMO. ("Code conforms to style guide" is not as useful for a contributor as "flake8 --docstring-convention=all passes").

While I would love this PR to contain the shorter version of the template, pointing at the docs, I propose we merge this long version to get the benefits of the new additions, and when the docs have been centralized a bit (PR incoming), that PR can also shorten this template.

[QuLogic]

Can you drop this either on master of your fork, or in a small repo, so we can see how it looks when opening a PR?

[brunobeltran]

Pushed to my master branch, so you should be able to open a PR against brunobeltran:master, to see what this looks like.

[QuLogic]

For example, one might try going here and clicking Create pull request. For me, the template is 8.5-9 scroll lengths long. If I resize the entry as tall as possible, it is 3 and bit pages. This may be a bit too long, even though every subsection is short. I would need to compare it to other's to get a feel for that.

It's too bad GitHub doesn't have multiple PR templates like for issues.

[jklymak]

A lot of work went into this. I'm somewhat against it, versus having the short checklist and a link to the actual docs explaining all this. But if those who like it actually like it, please merge. If you don't actually like it, lets close it?

[brunobeltran]

Thanks for bringing this to my attention again @jklymak.

I also think the bulk of the improvements that I'm starting to do here should go directly into the dev docs instead.

Closing in favor of #18341, which accomplishes the original goal without all the bloat that grew here (which I should also pull out into a proper PR of the dev docs).