-
Notifications
You must be signed in to change notification settings - Fork 3
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
Create pull_request_template.md #430
Conversation
Co-authored-by: Joe Flack <[email protected]>
Co-authored-by: Joe Flack <[email protected]>
Co-authored-by: Joe Flack <[email protected]>
Co-authored-by: Joe Flack <[email protected]>
Co-authored-by: Joe Flack <[email protected]>
.github/pull_request_template.md
Outdated
- [ ] Reviewed | ||
- Has been sufficiently reviewed by at least one review from a different team member of the Mondo Technical team. | ||
|
||
## Overview |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
## Overview | |
## Overview | |
<!--- A few sentences describing changes / what problem is solved. Should describe the expected result of the changes. If applicable, point out the main change that solved the problem, e.g. fixed bug in method xyz. --> |
I tried to select multiple lines to suggest replacement of multiple lines (step 4 in this guide from GitHub), but it didn't work. My suggestion here is to replace the whole "Overview" section with this one comment.
Rationale:
As I mentioned in this comment:
Our new suggested template suggests listing changes as bulleted list. But in practice, my preferences is a short paragraph (2-3 sentences). Anything more than that I usually put in an "additional information" section. And I actually have for a long time included a bulleted list of changes. However, I do this in a "Changes" or "Updates" section, and all that I do there is simply copy/paste my commit message(s).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The overview should be a few sentences that describes what problem is being solved with this PR. It's fine to refer back to the issue for more detailed information, but the comment in the PR should be clear enough to understand the problem.
The Overview should also point out what the major change was to solve the problem, e.g. fixed bug in method xyz or updated the make statement for goal 123.
The Overview should also point out what is the expected result of the change of the code fix, e.g. when sh run.sh make build-mondo-ingest
is run on the branch what is the new behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea agreed with that @twhetzel. @joeflack4 can fold this into your suggestion above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Overview should also point out what the major change was to solve the problem, e.g. fixed bug in method xyz or updated the make statement for goal 123.
I agree with that when it is applicable. Just spitballing, I think PRs have been small enough for single changes like these 25% of the time or so.
Yeah I'll go ahead and update the code suggestion there momentarily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unresolving conversation which was automatically closed by github.
Action items:
- add @twhetzel suggestions in a compressed form into the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright done, feel free to commit the suggestion if you guys like it!
Co-authored-by: Joe Flack <[email protected]>
Co-authored-by: Joe Flack <[email protected]>
Co-authored-by: Joe Flack <[email protected]>
I made a gist to suggest a new format to make it more clear the yes/no answers. |
Super cool Trish, I like it |
Regarding the gist, I like it too. Maybe we can make a new commit here for the next evolution of this template. The essential difference I glean is the difference of 1 checkbox per criteria, or 2+ checkboxes for options. I added a comment as well as an alternative evolution to the gist. |
Thanks for comments on the gist. I wasn't sure if this would be too long/drastic a change, but since y'all are ok with this I have committed this change and will merge. |
Fixes #361.
This PR: