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

Update Pull Request Template [ci-skip] #45495

Merged

Conversation

okuramasafumi
Copy link
Contributor

Summary

This change aims to help contributors to write better Pull Request description, especially new contributors.
From my experience new contributors cannot tell if they can discard a template even when they just fix a typo, so here it's clearly documented that it's discardable.

Copy link
Member

@eileencodes eileencodes left a comment

Choose a reason for hiding this comment

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

Thanks for these changes! I've made some suggestions.

.github/pull_request_template.md Outdated Show resolved Hide resolved
.github/pull_request_template.md Outdated Show resolved Hide resolved
.github/pull_request_template.md Outdated Show resolved Hide resolved
.github/pull_request_template.md Outdated Show resolved Hide resolved
.github/pull_request_template.md Outdated Show resolved Hide resolved
these bugs have open GitHub issues, be sure to tag them here as well,
to keep the conversation linked together. -->
This template aims to help contributors to write better descriptions.
Feel free to discard it if you need to.
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this needs a bit more. How about something like:

The following template aims to help contributors write a good description for their pull requests. We'd like you to provide a description of the changes in your pull request (ie bugs fixed or features added), motivation behind the changes, and complete the checklist below before opening a pull request.

Copy link
Member

Choose a reason for hiding this comment

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

I think this really needs to be in a comment block too... if not, it will get left untouched often, and every PR notification will end up starting with the same words.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eileencodes @matthewd I put them into a comment. I also added some more comments for motivation section.

Copy link
Member

@eileencodes eileencodes 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! I left one comment, then can you squash your commits into one? Thanks!

.github/pull_request_template.md Show resolved Hide resolved
The major change is to add checklist so that it's easier to be read
than comments.
Also, `Summary` is not clear about what should be written, so here
it's changed to `Motivation` and `Detail`.
Finally, it clearly says they can discard this template if they need to.
@okuramasafumi okuramasafumi changed the title Update Pull Request Template Update Pull Request Template [ci-skip] Jul 14, 2022
@eileencodes eileencodes merged commit f9030fe into rails:main Sep 8, 2022
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.

None yet

3 participants