clean up github PR form template

[Sevans711]

Closes #1595

Overview

Fixes/improves a variety of outdated/confusing aspects of the default PR form, as noted in #1595:

1. Comment at the top now properly states how to link multiple issues being closed (cannot just do "closes #XXX, #YYY" as previously implied; must repeat the keyword each time, such as "closes #XXX, closes #YYY").
2. Comment at the top now clarifies it is not necessary for PRs to fully close an issue, can instead write something like "Related to #XXX", or "Fixes part of #YYY but does not fully close it."
3. In "Expected Usage" comment, changed "If you are adding a feature into the Internal API, please produce a short example of it in action." to "If this PR adds a new feature, please provide a short example of it in action."
4. Clarified "(if you have label edit permissions)" on the "Added appropriate labels" checklist item
5. Removed reference to nonexistent "docs/interal_api/index.rst"; the checklist item now reads: "Internal (private) function names start with an underscore (_)"
6. Replaced reference to nonexistent "docs/user_api/index.rst"; the checklist item now reads: "User (public) functions have been added to docs/api.rst"
7. Clarified that new example notebooks are not limited to "docs/examples/", they should instead be added to one of the following, depending on example type: "gallery: docs/examples/; guide: docs/user-guide/; quickstart: docs/getting-started/". Also clarified that adding entries to "gallery.yml" only applies for gallery-type examples.
8. Replaced reference to nonexistent "examples.rst". The relevant checklist item now reads: "New notebook examples referenced in appropriate .rst file (gallery: docs/gallery.rst; guide: docs/userguide.rst; quickstart: docs/quickstart.rst)"
9. Rephrased final comment (in "PR Etiquette Reminders") to avoid implying PR submitter needs to request reviewers.
10. Fixed minor typos (e.g., "An issue is linked created and linked")

PR Checklist

General

• An issue is linked created and linked
• Add appropriate labels
• Filled out Overview and Expected Usage (if applicable) sections

[Sevans711]

Should we take this opportunity to add something to the "Testing" part of the PR checklist like "If this PR should improve memory usage or runtime efficiency, at least one benchmark demonstrates expected improvements (add a new benchmark if necessary)"?

[dylannelson]

Interesting and helpful finds. Some I didn't even notice when reading through before this, ha
One thing I'm curious about, the note
- [ ] Added appropriate labels (if you have label edit permissions)
does the "if" part mean that some users can't add labels (including existing ones) or they can't make new labels from scratch?
If it's the first, why might some users not be able to add labels at all (I imagine this isn't the case). If it's the second, is this simply a reference to the case where they may not be able to add an "appropriate" label but also can't make one of their own?

[Sevans711]

Thank you for taking a look!

One thing I'm curious about, the note - [ ] Added appropriate labels (if you have label edit permissions) does the "if" part mean that some users can't add labels (including existing ones) or they can't make new labels from scratch?

It is the former! I believe you need to be granted some nonzero level of permissions for directly editing the uxarray repository in order to actually assign labels to your PR. But, you can certainly open a PR from a fork, even without any permissions at all. For my first uxarray PR I didn't have permissions and opened it from a fork, and I remember being a bit confused about whether I should check this box, and whether I was supposed to be able to add labels somehow but just couldn't figure it out, when in fact you need some edit permissions for it.

Maybe we can workshop the phrasing a bit to come up with something that clarifies this, while also not being too vague? Perhaps something like "Added appropriate labels (if you have uxarray edit permissions)" would be clearer?

[dylannelson]

Oh that's interesting, I would have been confused too. Since some of the other checks have much more added context now, it may help to also add this context too (while also covering the case where they want to check the box but couldn't add labels):

- [] Added appropriate labels, or were unable to add labels (users can't add labels by default)

A bit wordy but may cover all the points of confusion? I avoided the word "edit" because that sounds like a higher level of permission that "using"/"adding", but it seems like the user can either do all or none so it may not matter much either way.

[erogluorhan]

Looks great overall, just a few inline comments

[erogluorhan]

Oh that's interesting, I would have been confused too. Since some of the other checks have much more added context now, it may help to also add this context too (while also covering the case where they want to check the box but couldn't add labels):

- [] Added appropriate labels, or were unable to add labels (users can't add labels by default)

A bit wordy but may cover all the points of confusion? I avoided the word "edit" because that sounds like a higher level of permission that "using"/"adding", but it seems like the user can either do all or none so it may not matter much either way.

How about "Added appropriate labels (if your repo permissions allow to do so)" @Sevans711

[Sevans711]

Thank you for your feedback and suggestions, @dylannelson and @erogluorhan. I have made changes accordingly; see 6580446:

• Changed phrasing to "Added appropriate labels (if your uxarray repo permissions allow it)".
• Deleted "If an entire section doesn't apply to this PR, comment it out or delete it" comment, to instead provide specific comments in each section. E.g., testing section starts with <!-- If this PR does not update any functionality or tests, remove this section (Testing) -->
• Changed phrasing to "All notebook examples cleared the output…"

Highlighting some lingering questions:

1. Should we take this opportunity to add something to the "Testing" part of the PR checklist like "- [ ] If this PR should improve memory usage or runtime efficiency, at least one benchmark demonstrates expected improvements (add a new benchmark if necessary)"? Also realizing now that I should have pinged @cmdupuis3 on this earlier.
2. Noticing that this bullet point in testing is extremely rigorous: - [ ] Tests cover all possible logical paths in your function and is followed by something far less rigorous: - [ ] Tests are not too basic (such as simply calling a function and nothing else). Does it actually makes sense to keep the extremely rigorous bar of "all possible logical paths"?

• (2a) If no, maybe we can drop it, or rephrase to something like "all major paths" instead?
• (2b) If we're keeping both bullets, I might move the "not too basic" bullet to be before the "all paths" bullet, if that sounds okay?

[cmdupuis3]

1. Should we take this opportunity to add something to the "Testing" part of the PR checklist like "- [ ] If this PR should improve memory usage or runtime efficiency, at least one benchmark demonstrates expected improvements (add a new benchmark if necessary)"? Also realizing now that I should have pinged @cmdupuis3 on this earlier.

Yeah, that would be helpful especially on my draft PRs.

[erogluorhan]

I could even approve it now, but a few quick comments below:

[Sevans711]

@erogluorhan Thank you again for reviewing! Pinging to let you know I made some minor changes (see replies to inline comments above, as well as small notes below). If you still approve there is no need to follow up here, just wanted to keep you in the loop.

Extra changes not included as replies above:

• moved "Tests are not too basic" checklist item to be above "Tests cover all paths" item.
• as per Slack conversation and my comment earlier in thread here, changed wording from "Tests cover all possible logical paths" to "Tests cover all major paths".

[dylannelson]

Read through it all again and think it's overall much more clear, covers additional meaningful reminders, and sounds like it could be understood by most users.