Skip to content
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

Tidy up pull request template #39909

Merged
merged 1 commit into from
Apr 29, 2020

Conversation

wapcaplet
Copy link
Contributor

@wapcaplet wapcaplet commented Apr 25, 2020

Summary

SUMMARY: Infrastructure "Improve usability of the pull request template"

Purpose of change

Whenever I open a pull request, there are several things about the template that feel unnecessarily difficult to me, and require rather more text-manipulation than I would like. Having recently learned that the triple-backticks around the SUMMARY line (and the issue-link example) are totally unnecessary, and having seen other contributors similarly confused, I thought we could all benefit from making this template better.

Describe the solution

Changes .github/pull_request_template.md:

  • Rephrased the "How to use" intro at the top
  • Moved URL links to a standalone line, so users do not have to select inside (...) to copy the URL
  • Rewrite the summary section for readability, with an eye toward keeping things simple and straightforward for new contributors
  • Reformatted all sections, removed all backticks and other unnecessary Markdown formatting
  • Redistributed whitespace for readability

Describe alternatives you've considered

Although this template is a Markdown file, the use of Markdown syntax inside <!-- ... --> comment blocks caused unnecessary clutter. The two URL links regarding the Changelog would be great if they were rendered and clickable - but here, if the user wanted to go to those URLs, they'd have to carefully mouse-select or keystoke inside the (https://...). Putting them on a standalone line makes it at least slightly easier to copy/paste them to view the two referenced documents.

In fact I'd like to remove almost all the Markdown formatting from these blocks and let line-separation and whitespace tell the tale. You don't really need to use Markdown syntax in your PR, so I would suggest it's better to keep it simple. Experienced contributors know how to Markdown already, and new contributors might appreciate a reduction in clutter and complexity in these sections.

All the above things turned out to be good ideas.

Rejected alternatives:

  • Including a literally correct SUMMARY line - this would confuse the summary validation parser in ways that are difficult to fix, and it is easy to work around through explanation
  • Including SUMMARY: None in the examples - while this summary is in fact recommended in the changelog guidelines for minor changes and bugfixes, I felt it would be more important to encourage new contributors to take the time to think of a meaningful summary, rather than propose "None" as the solution to having to think of one.

Additional context

I appreciate all the feedback on this, since it is such a high-visibility feature.

@wapcaplet
Copy link
Contributor Author

wapcaplet commented Apr 26, 2020

Direct link to the raw template for easier viewing: https://raw.githubusercontent.com/CleverRaven/Cataclysm-DDA/f2f131ebc4330585a973577bfcdc554b1b2c4550/.github/pull_request_template.md

I will leave this in [CR][WIP] for another day or two, but I am satisfied calling it my final draft pending further review/suggestions. (I will rebase it once more before taking off the WIP tag).

@jbytheway
Copy link
Contributor

Is it worth mentioning the option of "SUMMARY: None"?

@wapcaplet
Copy link
Contributor Author

Is it worth mentioning the option of "SUMMARY: None"?

I considered it, but had some doubts. Since the template is mostly oriented towards new users, I wanted to encourage the inclusion of meaningful summaries, and try to prevent anyone abusing SUMMARY: None simply because it's easier to write and get to pass validation.

I also didn't want to include it as an example, since it doesn't follow the same rules as the other categories (having no "quoted string" part), and would add complication that again I think is more likely to confuse new contributors.

More experienced contributors (and those who follow the link and read the changelog summary doc) have a better sense of when SUMMARY: None is appropriate.

I'm certainly willing to reconsider, but these reasons are why I have chosen to leave it out until now.

.github/pull_request_template.md Outdated Show resolved Hide resolved
@wapcaplet wapcaplet requested a review from kevingranade April 28, 2020 03:06
@wapcaplet
Copy link
Contributor Author

Sorry Kevin, I did not mean to hit the "request review" button there. Github is still showing your "change requested", so maybe it's just confused (or I am). Anyway this is about due for a rebase, coming shortly.

This commit changes the Github pull request template to:

- Remove superfluous Markdown formatting in comment blocks
- Move URLs to separate lines to facilitate copy/paste
- Clean up the Summary instructions and try to explain better
- Simplify the Purpose section with less emphasis on issue links
- Describe alternatives in alternative ways
@wapcaplet wapcaplet force-pushed the cleaner-pr-template branch from 0cfe51a to 5d59dc1 Compare April 28, 2020 03:44
@wapcaplet wapcaplet changed the title [CR][WIP] Tidy up pull request template Tidy up pull request template Apr 28, 2020
@kevingranade kevingranade merged commit bc87b3d into CleverRaven:master Apr 29, 2020
@wapcaplet wapcaplet deleted the cleaner-pr-template branch May 6, 2020 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants