github: Add pull request template. #1071
Conversation
There was a problem hiding this comment.
Thanks for getting this done, Neil. It already looks amazing. 👍
This is going to be very helpful, particularly, for new contributors. It will also be helpful for reviewers in getting to know what the PR does (abstractly), and how ready it is - is it properly tested, does the linting pass are the test added yet. Reviewers can also provide suggestions for any "Notes/Concerns" that the author has mentioned in the PR template without looking at the code - if possible too.
.github/pull_request_template.md
Outdated
| Overall description goes here! | ||
|
|
||
| - [ ] New Feature? | ||
| - [ ] Fixes Bug? [Fixes <issue>; Partially <issue> | Fixes point <n> of <issue> | Resolves an issue that's not filed] |
There was a problem hiding this comment.
Perhaps also include the discussion link, if there's no issue filed for the fix?
There was a problem hiding this comment.
Good point, though I think that's a separate entry to suggest including.
.github/pull_request_template.md
Outdated
| - [ ] Tests adapted (if necessary) | ||
| - [ ] Tests added/changed for any new behavior |
There was a problem hiding this comment.
Does the "adapted" and "changed" carry the same meaning here? Then we could remove the first check-box?
There was a problem hiding this comment.
The intent is to ensure that existing tests still pass, via slight changes if necessary - which is distinct from covering different new behavior. Perhaps the two should be explicitly "Existing tests pass (adapted if necessary)" and "New tests added (if new behavior)". Maybe they're too long though.
There was a problem hiding this comment.
Maybe we could follow the same wording as in the commit style guide?
Tests added (new feature)
Tests updated (refactor/feature that needs minor amending in existing tests)
There was a problem hiding this comment.
@Rohitth007 I think that'd require the contributor to explicitly write details in parenthesis? I believe we're keener on the tests in the PR in general, and specific additions/amendments are addressed in commits in any case.
There was a problem hiding this comment.
@Ezio-Sarthak No, I just used the parenthesis for explaining added vs updated here
.github/pull_request_template.md
Outdated
| <!-- See https://github.com/zulip/zulip-terminal#commit-style --> | ||
| **Commit flow** <!-- if more than one commit; add/delete/fill-in as appropriate --> | ||
| * first commit doing some thing |
There was a problem hiding this comment.
Although I've used this field in some of my own PR's, I wonder if it's really necessary?
The "Commit flow" is essentially just the commit headers, as we have documented in that link, which can be easily accessed from the commits tab at the top?
There was a problem hiding this comment.
I agree the commit list can be seen in the PR itself - and does change over time, often, and it's not necessary to keep updating the text here as that happens.
I do feel we could benefit from having something here though - maybe others would like to weigh in.
There was a problem hiding this comment.
Even I feel it's redundant sometimes. Maybe it's could be summarized in the summary section if needed, like the first few are related to this, the others are related to that and so on.
We could always implement and see how people use it.
There was a problem hiding this comment.
@Rohitth007 That's what the examples are intended to indicate, ie. that it's a summary. I meant to include that they're only ~ commit titles, but I don't want to motivate just copy and pasting those either.
.github/pull_request_template.md
Outdated
| * maybe multiple commits doing similar things | ||
| * ... | ||
|
|
||
| **Notes/Doubts/Concerns/Questions** <!-- if any; add/delete/fill-in as appropriate --> |
There was a problem hiding this comment.
This field is great. 👍
Can we simplify the title to just "Notes/Concerns"?
Doubts/Concerns/Questions sounds pretty similar to me.
There was a problem hiding this comment.
I agree 4 is too many. Maybe just "Notes" and push the possible content into the examples.
Of course the titles are editable/removable, but people may not think that, so simple titles may be easier as users are more likely to think to at least fill in their own content I hope :)
|
I'm more inclined to remove the first 3 checkboxes as GitHub includes them as a list of tasks - which they are not. I'm not sure there's an easy alternative, but we can simply hint at these elements in the description prompt, or in the commit flow perhaps too? |
neiljp
force-pushed
the
2021-07-07-PR-template
branch
from
0d30129 to
3b32447
Compare
There was a problem hiding this comment.
@neiljp Thanks for working on this! 👍 This looks great. I have left a minor in-line comment.
.github/pull_request_template.md
Outdated
|
|
||
| **Interactions** <!-- if any; add/delete/fill-in as appropriate --> | ||
| - Waiting on other PR(s)? What numbers? | ||
| - Blocks other PR(s)? What numbers? |
There was a problem hiding this comment.
Should these two points exist as a comment like the above two sections?
neiljp
force-pushed
the
2021-07-07-PR-template
branch
from
3b32447 to
df4e74e
Compare
What does this PR do?
Adds a pull request template, like I'm using here :)
Take a look at the raw and rendered versions to get a full idea of the contributor experience.
Tested?
Notes/Doubts/Concerns/Questions
This is more detailed than others I've seen, eg. https://raw.githubusercontent.com/zulip/zulip/master/.github/pull_request_template.md
Do we need the top title? Or feature/bug boxes? (except for suggesting what it may fix?)
Do the testing boxes help? Do they just take up space?