PR: Greatly simplify and clarify Github pull request template

[CAM-Gerlach]

Pull Request Checklist

• Read and followed this repo's Contributing Guidelines
• Based this PR on the latest version of the correct branch (master or 3.x)
• Followed PEP 8 code style
• Ensured this pull request hasn't eliminated unrelated blank lines/spaces,
  modified the spyder/defaults directory, or added new icons/assets
• Described the changes and the motivation for them below
• Noted what issue(s) this pull request resolves, creating one if needed
• Included a screenshot, if this PR makes any visible changes to the UI

Developer Certificate of Origin Affirmation

By submitting this Pull Request or typing my name below, I affirm the
Developer Certificate of Origin
with respect to both the content of the contribution itself and this post,
and understand I am releasing it under Spyder's MIT (Expat) license.

I certify the above statement is true and correct: CAM-Gerlach

Description of Changes

Per the suggestions of @jnsebgosselin , I've greatly simplified the PR template, reducing its total size by ~40% (by character count).

• Split the PR checklist into two parts:
  • One with a condensed version of the basics that apply to every PR on submission (Contrib Guide, correct branch, PEP8/PEP 257, no-nos) and make that all just one checkbox,
  • One with the other three (Docstrings, Unit tests, screenshot) as individual bullets that can be removed as necessary
• Cut down excess text throughout the rest of the template

Issue(s) Resolved

Fixes #8270

[jnsebgosselin]

I do not see the benefit of adding that info in a checkbox :

* [ ] I have read the [Contributing Guidelines](
      https://github.com/spyder-ide/spyder/blob/master/CONTRIBUTING.md),
      and followed the Spyder's conventions:
      - [General](https://github.com/spyder-ide/spyder/wiki/Dev:-Coding-Style)
      - [PEP 8](https://www.python.org/dev/peps/pep-0008/)
      - [PEP 257](https://www.python.org/dev/peps/pep-0257/)

The only purpose I can think of for this is to be able to play the PR police... I agree this is useful when someone contributes for the first time, but after that, the checkbox becomes meaningless. I doubt that having to check this on every PR really helps long-time contributors and devs to remember that they needs to follow PEP8 and PEP257 in their PR....

So, I would replace that by a simple sentence written as a MD comment. In addition, we can use Probot to provide more infos to first time contributors.

[jnsebgosselin]

I think the checkboxes in the "Description of changes" section should be written as a series of commands, like:

- [ ] Write at least one-line docstrings (for any new functions).
- [ ] Add unit test(s) covering the changes (if testable)
- [ ] Include a screenshot (if affecting the UI)

But I'm not sure the "Write at least one-line docstings (for any new functions)" has its place there. I think this is included as part of the "we need to follow the PEP 257 Guidelines" specification. It is in the same league as the 4-space indent specification IMO.

[jnsebgosselin]

By submitting this Pull Request or typing my name below, I affirm the
[Developer Certificate of Origin](https://developercertificate.org/)
with respect to both the content of the contribution itself and this post,
and understand I am releasing it under Spyder's MIT (Expat) license.


<!--- TYPE YOUR GITHUB USERNAME AFTER THE FOLLOWING STATEMENT ---!>
I certify the above statement is true and correct:

This confusing to me. It says By submitting this Pull Request OR typing my name below. So, why are we requested to write our name below, if this can be done simply by submitting the PR? Would it be possible to replace that by :

By submitting this Pull Request, I affirm the
[Developer Certificate of Origin](https://developercertificate.org/)
with respect to both the content of the contribution itself and this post,
and understand I am releasing it under Spyder's MIT (Expat) license.

[jnsebgosselin]

Or, regarding the #8272 (comment) I made above, instead of writing down our names, would it be possible to add this in a checkbox instead? I think in that case, that would be very meaningful.

[CAM-Gerlach]

So, I would replace that by a simple sentence written as a MD comment.

Okay, done.

I think the checkboxes in the "Description of changes" section should be written as a series of commands

Okay, but is there a particular reason/justification for that, other than just the habit of writing commit messages that way? Neither are all that decisive, but I see two conceptual reasons why past tense makes sense here:

• Given they are directly under the heading "description of changes", in the past tense they are in same form as one would conventionally describe other things accomplished in the PR, like "I've greatly simplified the PR template" whereas they would sound out of place there as imperative
• In the past tense, they represent things that have been done, which is what they are supposed to mean if checked and what people are checking to assert, while in the imperative they represent commands—they also are in this case, but it comes across softer and less demanding to avoid the imperative tense where not necessary.

I think this is included as part of the "we need to follow the PEP 257 Guidelines" specification.

That was my initial thinking too, but the reason I put it there was that it fell under the "optional things that might not be included in the initial submission", and the fact that PEP 257 only says that public functions and methods "should" have docstrings, as well as it being more specific about the nature of docstrings to include. I can remove it since that's not a very strong justification, but that leaves us with only two list items which looks a little silly.

[jnsebgosselin]

Okay, but is there a particular reason/justification for that, other than just the habit of writing commit messages that way? Neither are all that decisive, but I see two conceptual reasons why past tense makes sense here:

* Given they are directly under the heading "description of changes", in the past tense they are in same form as one would conventionally describe other things accomplished in the PR, like "I've greatly simplified the PR template" whereas they would sound out of place there as imperative

Ok I understand. I was seeing this more like a "TODO" list. Like this PR for example #8192.

* In the past tense, they represent things that have been done, which is what they are supposed to mean if checked and what people are checking to assert, while in the imperative they represent commands—they also are in this case, but it comes across softer and less demanding to avoid the imperative tense where not necessary.

This makes sense too. I was not seeing this as a list of things we are asking contributors to do, but more as a tool for the contributors to use to structure and document the progress in their PR.

So basically, I think both options as correct. It depends if you see the PR template as a tool for the developers or as a tool for guiding first time contributors.

Edit: I'm not sure what I like the best. I will need to think about it. So do as you wish.

[jnsebgosselin]

Ok, regarding my #8272 (comment), I think I like better the past tense finally, so let's keep it that way. Forget what I said.

[CAM-Gerlach]

Also, @CAM-Gerlach, you are free to disagree and argue with my propositions. I do not mean to impose my vision here.

You do know whom you're asking that of, right? :P If there is one thing I am infamous on this repo for, it is writing long and rambling replies disagreeing and arguing with even the most trivial of propositions.

So, why are we requested to write our name below, if this can be done simply by submitting the PR?

That's a good point, and one I've considered at length multiple times. Singing it positively affirms people have actually seen, read and agree to the DCO, both making sure they understand what they agreeing to and they it is clear that they are actually agreeing to it in an enforceable way. Its the reason almost all websitse or installer with a license/ToS that you have to agree to at some point makes you actually click "Agree" or check a box rather than just saying "by creating an account" and leaving it at that, or even pre-checking the box/selecting "Agree" by default, at least whenever they can. However, including the "or submitting the PR" covers us in case they don't, and we forget to ask them to, or they try to sneakily remove it and claim that means they don't agree anymore. By including both, we're covered in all reasonable scenarios.

would it be possible to add this in a checkbox instead? I think in that case, that would be very meaningful.

I considered that, but a checkbox is much more easily "mutable" and less ambiguous than a written name, which unlike a checkbox requires a specific and deliberate sequence of actions that cannot be construed to be an accident. Furthermore, typing one's name is considered to be legally equivalent to "signing" something, in this case the DCO, while there is no such strong legal precedant behind checking a Github checkbox. If we're going to include this, its worth spending an extra second or two and doing it right (of course, the best way would be having all contributors sign their commits with git commit -s, but if they forgot to or didn't know to even one time, they would have to rebase their entire PR and resign every commit that they didn't, which is a comparatively huge burden especially for newer and less experienced contributors).

[CAM-Gerlach]

So basically, I think both options as correct. It depends if you see the PR template as a tool for the developers or as a tool for guiding first time contributors.

Indeed, two ways of looking at the same thing. It certainly can be seen as a todo list. Ultimately both reasons were pretty weak, the first based on the context of the heading they were under, and the second optimizing to be a little more gentle on new contributors while not taking anything really away from current ones.

[CAM-Gerlach]

Hey @jnsebgosselin , are there more changes you'd like to make, or do you still need time to look over it?

[jnsebgosselin]

This is IMO a very good step towards the right direction. I'm happy with the work we have done here.

I left some minor comments.

[CAM-Gerlach]

@jnsebgosselin Thanks for all the catches; my brain was clearly turned off last time missing all that, since even I saw it right away opening up the file today.

[jnsebgosselin]

[jnsebgosselin]