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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify Pull Request template #39229

Merged
merged 6 commits into from Mar 8, 2022
Merged

Simplify Pull Request template #39229

merged 6 commits into from Mar 8, 2022

Conversation

priethor
Copy link
Contributor

@priethor priethor commented Mar 4, 2022

What?

This PR tries to simplify the pull request template for a better contributor experience.

Why?

Currently, understanding the context and purpose of some PRs can be challenging.

One reason is that the PR template is quite verbose and is not always followed. For example:

  • The "types of changes" section can be described with labels.
  • The checklist is often omitted, as some steps don't apply or can be checked by linting tools.

On the other hand, basic information like the purpose of the PR and why it's necessary is often missing; sometimes, the context is in another issue and hard to find among a long list of comments.

Simplifying the template with a more direct, less frictional form, might help contributors provide more context and homogenize PRs descriptions.

How?

By:

Testing instructions

  1. Head to this repository
  2. Create a new PR and check how the proposed changes would look if approved

This very PR is created following the template, too, so this is how it would look like 馃檪

Screenshots or screencast

PR-ception
image

@priethor priethor added the [Type] Project Management Meta-issues related to project management of Gutenberg label Mar 4, 2022
@priethor priethor changed the title Update PULL_REQUEST_TEMPLATE.md Simplify new PR template Mar 4, 2022
@priethor priethor changed the title Simplify new PR template Simplify Pull Request template Mar 4, 2022
@priethor priethor marked this pull request as ready for review March 4, 2022 19:26
@priethor priethor requested review from a team, mcsf, Mamaduka, luisherranz, mtias, youknowriad, gziolo, noisysocks, getdave, annezazu and ellatrix and removed request for a team March 4, 2022 19:27
@alexstine
Copy link
Contributor

@priethor The PR template looks the same on your repo. Should I be selecting a specific branch?

I am not sure removing the whole checklist is a good idea. The ones that are not caught by tests always such as accessibility should stay as a reminder. Let's face it, no one reads docs anymore. 馃檪

@priethor
Copy link
Contributor Author

priethor commented Mar 4, 2022

Thanks for chiming in, @alexstine!The template in my repo is indeed the same; the idea is that folks testing the proposed format can see how it looks in my repo. Let me rephrase that in the testing instructions above.

As far as the checkbox are concerned, I agree that completely removing them is not the best solution. However, we also need to face that the checklist is often ignored, so maybe we should try to find a sweet spot and only leave the most impactful checkboxes, even if it is a single one saying "I have read the contributing docs". We need to balance helping less frequent contributors remember contributing guidelines and not adding friction to regular contributors.

@youknowriad
Copy link
Contributor

Personally I like this a lot.

I agree that Testing, Accessibility, Standards, Mobile, Public APIs (crucial as we can't break backward compatibility) are important, but I don't think the PR template is the best place to introduce contributors into all these requirements. A simple link to the editor handbook is probably better and invite folks (if they are new) to learn more about all the requirements of the repo that can't be summed up in a small check list.

I think it's very hard to come up with the best PR template that doesn't discourage contributors or just make them ignore it. (As you can see in most of Gutenberg PRs), so I think we should iterate and see what works best, we know for certain that the current one doesn't work entirely.

Answering the "Why", "How" and "What" can be a potential solution way to encourage contributors to add more context about their PRs, avoid the hidden assumptions and help them get more reviewers and feedback because it removes a burden from the reviewer to try to understand the reasoning and purpose of the PR. The reviewer doesn't need anymore to jump from linked issue to another to be fully operational on the PR.

馃憤 for me (I'd love if we can add just a small link to the contributions section of the handbook).

@mcsf
Copy link
Contributor

mcsf commented Mar 6, 2022

It鈥檚 a 馃憤 from me too, with the same note about linking to the handbook.

@Mamaduka
Copy link
Member

Mamaduka commented Mar 7, 2022

I like the new proposed template. Thanks, @priethor 馃憤

We can hide handbook links using HTML comments. Those are primarily useful for new contributors and not for reviewers. This is what the core repo does - https://raw.githubusercontent.com/WordPress/wordpress-develop/trunk/.github/pull_request_template.md

@gziolo
Copy link
Member

gziolo commented Mar 7, 2022

I love the direction. In practice, as of today, the "Description" is treated like "What" from the new proposal. The new template proposed should help to gather more details. We too often miss the "Why" part which is crucial for understanding the reason was opened in the first place. The "How" section is nice to have as it can be digested from the code, but it is a great addition as it acts as a condensed summary of all code changes included. Maybe we should also ask there what alternatives were considered?

In my experience the checklist is problematic. An example from the PR #39244 I opened today that changes development tool:

Screenshot 2022-03-07 at 08 18 03

We need to find a better way to educate contributors about guidelines rather than leaving a confusing list of items to take into account that isn't fully applicable for a big number of open PRs. Can we leave a few links as an HTML comment as suggested by @Mamaduka for now? In the long run, we should look into building a GitHub action bot that tries to detect the type of changes and guides them with a call for action, for example:

  • add labels to PR
  • reference a related issue
  • enforce documentation changes and CHANGELOG entries for new APIs

Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in favour of this change. Anything that encourages better PR descriptions and context has got to be a step forward. Thank you for proposing 馃憦

@priethor
Copy link
Contributor Author

priethor commented Mar 7, 2022

Thanks, everybody for the feedback 馃檹

It is important that contributors follow those guidelines but it is also clear that the current checklist isn't achieving its purpose, so we can try simplifying it to links as some of you have suggested. I've added to the top the links from the checklist but combined the coding standard ones in a single item. We can always iterate later or roll back this PR, if necessary.

Also, part of the checklist items refer to the React Native files, so I'd like to get some feedback from the mobile folks. Currently, it is not very clear for new contributors not familiar with React Native what the expectations around that checklist item are, and the link provided in it currently doesn't work. @hypest do you have any suggestion on where to link new contributors so that can very easily understand what/if they need to have anything into account mobile-related?

Copy link
Contributor

@gwwar gwwar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the idea here @priethor!

Copy link
Contributor

@alexstine alexstine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Can always improve based on feedback. Having one central contribution resource would be awesome. 馃憤

Link to the Contributing Guidelines
Extend the contributing guidelines by adding coding standards and accessibility testing.
@annezazu
Copy link
Contributor

annezazu commented Mar 8, 2022

As others have mentioned, I'd love to find a way to have a GitHub action bot guide the PR author as needed based on a few factors. Otherwise, looks good to me and I'm keen to see how it'll change things!

The only other thing that comes to mind is that I love seeing when something is clearly marked as an experiment (like this recent PR #39243) and think it would be neat to encourage that behavior. Likely outside of this scope though.

@talldan
Copy link
Contributor

talldan commented Mar 8, 2022

I like this a lot, thank you for making this change.

The 'Why?' and 'How?' sections are good additions. This is something that often wasn't factored into PR descriptions, but I think it's important to capture the motivation and intent behind a change.

I'm also in favour of removing the checklist. I think there were too many items and the more there were, the more the value of each one was diluted. I also found many of the checklist items were irrelevant for particular types of PRs.

@hypest
Copy link
Contributor

hypest commented Mar 8, 2022

Also, part of the checklist items refer to the React Native files, so I'd like to get some feedback from the mobile folks.

Thanks for the ping and request for input @priethor !

Currently, it is not very clear for new contributors not familiar with React Native what the expectations around that checklist item are

Ah, happy to collab on rephrasing if needed! To elaborate on what the current checklist prompt is about, it's pretty much what is mentioned on the item: (please manually search all *.native.js files for terms that need renaming or removal). Essentially, we ask folks to ensure that functions/variables/classes/etc that the PR renames or removes are mirrored on any .native.js file that uses them, and mark the checklist item as "done", always, even if they think is not applicable.

For context on the why this item, typically, refactoring tools are ignoring those files and is easy for the PR author to miss refactoring them. It has happened a few times in the past that's why we introduced that checklist item/request.

To also add "what that checklist item is not", the item is not asking folks to setup an React Native tooling or to test out the changes on native mobile. That'd be highy valuable but not in the scope of the checklist item.

Let me know if that helps clear up the expectations, happy to elaborate!

and the link provided in it currently doesn't work. @hypest do you have any suggestion on where to link new contributors so that can very easily understand what/if they need to have anything into account mobile-related?

Ouch, sorry we missed updating the link in the past. The relevant link now is: https://github.com/WordPress/gutenberg/tree/trunk/docs/contributors/code/react-native. We can open a PR to fix it up anyway, outside this PR here.

@priethor
Copy link
Contributor Author

priethor commented Mar 8, 2022

Thanks, @hypest; I've added the corresponding reference to the Contributor Guidelines linked on top of the PR template.

As clear as the React Native contributor guidelines are for React Native contributors and other experienced Gutenberg contributors, it might not be so for new contributors missing the broader context. I would encourage iterating on these guidelines to highlight even more contributor expectations around checking the .native.js files, too.

@priethor priethor merged commit 6bcacdd into trunk Mar 8, 2022
@priethor priethor deleted the PR-template-update branch March 8, 2022 13:16
@github-actions github-actions bot added this to the Gutenberg 12.8 milestone Mar 8, 2022
@priethor
Copy link
Contributor Author

priethor commented Mar 8, 2022

I like how @gwwar is integrating the screencast in the What section in this PR, because the video is indeed showing what the PR does. Does it make sense to remove the Screenshot section and just embed image/videos under that What/Why ones?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Project Management Meta-issues related to project management of Gutenberg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet