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

PR template #70

Merged
merged 5 commits into from
Jun 20, 2023
Merged

PR template #70

merged 5 commits into from
Jun 20, 2023

Conversation

sdhoyt
Copy link
Contributor

@sdhoyt sdhoyt commented Jun 2, 2023

This is a proposed PR template with pre-defined sections that should help the reviewers when reviewing each PR. This is more of an initial proposal, so I'm very open to modifications/additions/deletions to the template.

Ironically, I don't think there's any point in using this PR template for this PR.

@codecov
Copy link

codecov bot commented Jun 2, 2023

Codecov Report

Patch coverage has no change and project coverage change: +16.23 🎉

Comparison is base (8d47522) 70.26% compared to head (9dcd44b) 86.49%.

Additional details and impacted files
@@               Coverage Diff                @@
##           development      #70       +/-   ##
================================================
+ Coverage        70.26%   86.49%   +16.23%     
================================================
  Files               21       21               
  Lines              955      955               
================================================
+ Hits               671      826      +155     
+ Misses             284      129      -155     

see 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@greglucas greglucas left a comment

Choose a reason for hiding this comment

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

Definitely 👍 Just some minor suggestions that you probably want to wait until others comment too. (Could get a lot of opinions this way too though!)

For the PRs, do you want to put the content section in comment blocks <!-- XXX --> so they don't automatically show up if someone doesn't delete those sections?

Here are a few others I found quickly if they also give any inspiration.
https://github.com/github/docs/blob/main/.github/review-template.md
https://axolo.co/blog/p/part-3-github-pull-request-template

Comment on lines 9 to 22
## New Files
- new file 1
- description of new file 1's purpose

## Deleted Files
- deleted file 1
- explanation for why file was deleted

## Updated Files
- updated file 1
- description of change 1 in file 1
- description of change 2 in file 2
- updated file 2
- descipriton of change 1 in file 2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
## New Files
- new file 1
- description of new file 1's purpose
## Deleted Files
- deleted file 1
- explanation for why file was deleted
## Updated Files
- updated file 1
- description of change 1 in file 1
- description of change 2 in file 2
- updated file 2
- descipriton of change 1 in file 2

I am not sure this much detail is needed unless you want to draw a reviewer's attention to something specific because these are all shown in the Files section of the PR, so it seems like more work for the author to list specific updates like this.

Sometimes if I want to draw attention to something specific I'll review my own PR and leave a comment at that line for the specific callout too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Greg. Maybe we could replace all the new/deleted/updated file sections with some kind of "description of changes" section. So just a general list of the things that were changed and why. That can be the spot for further explanation if needed. I also prefer to keep the description general and call out specific things via comments

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, reading through your review that uses this format, maybe it is useful to have the more detailed template, which then can be slimmed down as needed. I guess if you find yourself writing a really long description, that might be a sign more than one PR is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the benefit to the file detail is it saves the reviewer time and better prepares them when they look at the file, since they will already know the purpose of those changes to that specific file instead of having to interpret it by reading the code. Plus the description provides an extra check to confirm that what the author thinks the changes are doing line up with what the reviewer sees the code doing. I can add a note that those bullets shouldn't be describing every line change, just the larger concepts behind each change. It is more work for the author, but it shouldn't take more than a few minutes unless your PR is way too large. Does anyone else have an opinion on this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm fine with that, people can always delete and change things when making the PR later if they really want. This is a template to work from and doesn't necessarily need to be rigidly followed.

Copy link
Contributor

@tech3371 tech3371 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!

@sdhoyt sdhoyt merged commit a495395 into development Jun 20, 2023
@sdhoyt sdhoyt deleted the pr_template branch June 20, 2023 19:10
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.

4 participants