chore: update pr template #16297
Open
chore: update pr template #16297
+18
−0
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
iurimatias
requested review from
a team,
0x-r4bbit,
virginiabalducci,
Cuteivist,
noeliaSD,
Khushboo-dev-cpp and
igor-sirotin
and removed request for
a team,
0x-r4bbit and
noeliaSD
Jenkins Builds
|
virginiabalducci
approved these changes
Sep 9, 2024
caybro
approved these changes
Sep 10, 2024
| - [ ] I have followed the architecture guidelines: [Architecture guidelines](../architecture-guide.md) | ||
| - [ ] For bug fixes, I have written a new unit test case to prevent reintroduction | ||
| - [ ] I have performed side impact testing for changes in shared components | ||
| - [ ] A reviewer has Tested the PR during review |
There was a problem hiding this comment.
these two might be better in a reviewer checklist, not sure if thats a thing though.
0x-r4bbit
reviewed
Sep 10, 2024
| - [ ] I have performed side impact testing for changes in shared components | ||
| - [ ] A reviewer has Tested the PR during review | ||
| - [ ] Given enough time for a thorough review | ||
|
|
There was a problem hiding this comment.
A little bit of feedback here:
- Keep checkbox labels short, it's too much processing to read all of them
- Example:
Has unit testsinstead ofI have written unit tests for status-go and QML changesHas user storiesinstead ofI have created stories for this feature/reworkHas storybook page (create if not existing)etc ...
- Example:
- The
I have followed the architecture guidelinesnot sure this adds a lot of value here. While some of the points above can act as reminders (don't forget to add tests!) this one is more blurry. - Maybe instead of
I have performed side impact testing...Add a section that says something like:
Side effects caused by these changes worth mentioning:
- The
A reviewer has tested this PR during reviewis something that should just be tackled by GitHub's "Request Review" feature, which implies the result in this checkbox automatically - The
Given enough time for a thorough reviewis far fro quantifiable. If you want to have this guaranteed, it probably makes more sense to agree on a rule like: "Leave PRs at least # days pending for review, don't merge before"
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does the PR do
update PR template to include some of the new guidelines