PR template: Make it better!

[9999years]

Closes #10575

The new PR template is short and sweet!

• PR templates will be read many times. PR authors will NOT see Markdown links rendered, so they add a lot of visual noise. Most links are therefore removed.

  • There's a balance here: A lot of these checks have additional detail in the CONTRIBUTING.md. Linking to a section can be helpful for new contributors, but if their eyes glaze over the whole item after deeming it too noisy, the CONTRIBUTING.md serves no benefit.
  • Most of the checks are pretty self-explanatory, so hopefully this is OK.

• Added checks for items that are listed in the "other conventions" but are handy to have explicitly: Haddocks are added, changelog entries are written, etc.

• Unified the two checklists; now, authors just have to decide if each check is relevant for their PR, rather than reading qualifications for which checklist to use.

• Would it be worth splitting the CONTRIBUTING.md up or publishing developer documentation explicitly?

  See the Rustc developer guide for a model: https://rustc-dev-guide.rust-lang.org/

[ulysses4ever]

I like it (esp. the <!---comment) although I'll be missing Templates A and B because it was really convenient to not have to decide what to do with all the unnecessary bullet-points for PRs that don't change the behavior.

Could we perhaps get best of both worlds by doing something like this:

- [ ] Is this a PR that fixes CI? If so, it will need to be backported to older cabal release branches (ask maintainers for directions).
- [ ] Commit messages are formatted nicely
- [ ] Post-merge: Backports for older Cabal release branches have been created
- [ ] Below bullet-points don't apply because this PR doesn't change Cabal's behaviour or interface
	- [ ] Tests have been added (Ask for help if you don’t know how to write them)
	  - [ ] Manual QA notes have been added
	- [ ] A changelog entry has been added in `changelog.d/pr-YOUR_PR_NUMBER`
	- [ ] Documentation has been updated
	- [ ] Haddock comments for new top-level definitions have been added
	- [ ] `base` and third-party library imports use qualified imports or explicit import lists

[9999years]

it was really convenient to not have to decide what to do with all the unnecessary bullet-points for PRs that don't change the behavior

I guess I've never really been clear on what "PRs that don't change the behavior or interface" means. Here's some edge cases I've been unsure about:

• A change to the type signatures in a module with Internal in the name that's listed in Cabal's exposed-modules, like Expand and unify --keep-temp-files #10292. We want tests and Haddocks for this, but maybe not a changelog entry in most cases...?
• A change to the command-line interface which is backwards compatible with the old interface but adds new (no-op) options to some commands, like Expand and unify --keep-temp-files #10292.
• A change to an error message to make the behavior clearer, like Show constraint sources in dependency solver errors #10524. We want all the checklist items for this, but it doesn't really change the 'behavior' in the sense that command-line output is auxiliary to what Cabal actually does.
• A change in implementation strategy that shouldn't be visible to users, like VCS: Don't run submodule commands unless necessary #10590. We want tests and changelog entries for this, but it is a purely internal change!

In all or almost all of these cases, I'd like the author to go through the checklist and decide what applies. If we can come up with some clearer guidelines for these checks, I'd be happy to change it, but I've found the status quo confusing enough that I've mostly just ignored the "PR that changes behavior" checklist.

[ffaf1]

The new template has good ideas, but overall I don't fancy it.

There are — broadly — two users we serve with the PR template.

The first one are newcomers. The ideal for this segment is a streamlined and clear contributing experience. Empirically, we received feedback in this regard, see #8511:

People keep complaining about our PR template in that it has a long set of checkboxes that are not necessarily relevant for small contributions, and those are usually made by newcomers, so they get especially confused. Maybe, we need another template: something like "Documentation" or "User-invisible change" without any checkboxes.

With the new template we have exactly the same “long set of checkboxes” people kept complaining about.
The lack of links too is not apt in my opinion. If I come around [ ] Manual QA notes have been added I would like a precise link on what I have to do, and not just a generic reference to a quite long CONTRIBUTING.md document.
“Not all of the checklist items will be applicable for your PR. Use your judgement.” has problems too, as a new contributor rightully has little idea on what is necessary and what is not. Of course he can ask but then what use is the template for?

For experienced contributors simply put, the template/s is a way to keep them honest. Having a list of checkboxes of which some can be skirted (if I understood correctly) will make the “x of y tasks” overview of the PR less useful than it is today.

So a simplification for the author, but at the cost of UX for the reviewer/maintainer.

The idea of using <!-- --> to hide part of the template is very good.

The best of both worlds would be separate templates for different needs (“Is this your first cabal contribution? Click this button!” “Is this a doc change?” etc.), but GitHub allows it for issues, while makes it clumsy for PRs.

[ffaf1]

There is a discussion on the matter (implement a template chooser) prompted by developers.
Alas, no feedback from GitHub.

[9999years]

@ffaf1 Would you please propose some changes? I'm happy to find a midpoint here.

“Not all of the checklist items will be applicable for your PR. Use your judgement.” has problems too, as a new contributor rightully has little idea on what is necessary and what is not. Of course he can ask but then what use is the template for?

I view the template as a starting point. There's always going to be edge cases, but it's a good overview of something like 90% of tasks for 90% of PRs. I'm not sure how to make the "x of y tasks completed" UI useful without providing multiple very granular checklists... which of course requires the PR author to use their judgement to determine which checklist is appropriate for their PR.

[ulysses4ever]

Here's my concern: the proposed version says: some things are optional, exercise your judgement for which things are and aren't. It doesn't try to help the author in making this judgement at all. While the currently deployed version has some guidance: it says that the shorter template is acceptable if this and that. This and that aren't exhaustive and may be vague as you rightly point out. But what we see in practice, many cases are easily solved using this guidance ("E.g. the PR only touches documentation or tests, does refactorings, etc.", I admit it's buried a bit deep down the text, and this could be improved). So, I think the proposed version should strive to do more in terms of guidance.

[ulysses4ever]

I guess I've never really been clear on what "PRs that don't change the behavior or interface" means. Here's some edge cases I've been unsure about: ...

That's the issue with the current template. Our view has consistently been that everything you mentioned is a user-visible change and requires Template A. It's a mistake that the constructive definition of what isn't a user-visible change (docs/tests/refactorings/CI) goes far below the preamble, as a part of Template B title. We should pull it up.

[ffaf1]

It doesn't try to help the author in making this judgement at all. While the currently deployed version has some guidance: it says that the shorter template is acceptable if this and that.

I second this.

In general we went away from a “long checklist/tick what is appropriate” setup to a “choose a template and complete it” for good reasons. I see benefits in improving the “select your template” process, I don't see much benefit in going back to a long list.

I feel I am not being constructive because I am resistant to this change.

It's a mistake that the constructive definition of what isn't a user-visible change (docs/tests/refactorings/CI) goes far below the preamble, as a part of Template B title. We should pull it up.

Agreed.