change: [M3-8857] - Update PULL_REQUEST_TEMPLATE (Part 2) #11236
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,15 +1,20 @@ | ||
| ## Description 📝 | ||
|
|
||
| Highlight the Pull Request's context and intentions. | ||
|
|
||
| ## Changes 🔄 | ||
|
|
||
| List any change(s) relevant to the reviewer. | ||
|
|
||
| - ... | ||
| - ... | ||
|
|
||
| ## Target release date 🗓️ | ||
|
|
||
| Please specify a release date (and environment, if applicable) to guarantee timely review of this PR. If exact date is not known, please approximate and update it as needed. | ||
|
|
||
| ## Preview 📷 | ||
|
|
||
| **Include a screenshot or screen recording of the change.** | ||
|
|
||
| :lock: Use the [Mask Sensitive Data](https://cloud.linode.com/profile/settings) setting for security. | ||
|
|
@@ -23,38 +28,53 @@ Please specify a release date (and environment, if applicable) to guarantee time | |
| ## How to test 🧪 | ||
|
|
||
| ### Prerequisites | ||
|
|
||
| (How to setup test environment) | ||
|
|
||
| - ... | ||
| - ... | ||
|
|
||
| ### Reproduction steps | ||
|
|
||
| (How to reproduce the issue, if applicable) | ||
|
|
||
| - [ ] ... | ||
| - [ ] ... | ||
|
|
||
| ### Verification steps | ||
|
|
||
| (How to verify changes) | ||
|
|
||
| - [ ] ... | ||
| - [ ] ... | ||
|
|
||
| ## As an Author, I have considered 🤔 | ||
|
|
||
| - 👀 Doing a self review | ||
| - ❔ Our [contribution guidelines](https://github.com/linode/manager/blob/develop/docs/CONTRIBUTING.md) | ||
| - 🤏 Splitting feature into small PRs | ||
| - ➕ Adding a [changeset](https://github.com/linode/manager/blob/develop/docs/CONTRIBUTING.md#writing-a-changeset) | ||
| - 🧪 Providing/improving test coverage | ||
| - 🔐 Removing all sensitive information from the code and PR description | ||
| - 🚩 Using a feature flag to protect the release | ||
| - 👣 Providing comprehensive reproduction steps | ||
| - 📑 Providing or updating our documentation | ||
| - 🕛 Scheduling a pair reviewing session | ||
| - 📱 Providing mobile support | ||
| - ♿ Providing accessibility support | ||
|
|
||
| <br/> | ||
|
|
||
| - [ ] I have read and considered all applicable items listed above. | ||
|
Comment on lines
+53
to
+68
There was a problem hiding this comment. Converted to bullet points instead of checkboxes and added a final checkbox for contributors to acknowledge the section. Do we feel okay about this? There was a problem hiding this comment. These are all good changes! I think we need a followup after opening a PR. Some suggestions after opening a PR (could be weighed by a larger group)
First item (<-- two spaces)
Second item
There was a problem hiding this comment. @abailly-akamai Thanks for the feedback! I just added this as a cafe item for Thursday. |
||
|
|
||
| ## As an Author, before moving this PR from Draft to Open, I confirmed ✅ | ||
|
|
||
| - [ ] All unit tests are passing | ||
| - [ ] TypeScript compilation succeeded without errors | ||
| - [ ] Code passes all linting rules | ||
coliu-akamai marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| --- | ||
|
|
||
|
Comment on lines
+70
to
+77
There was a problem hiding this comment. This was a suggestion from Retro. Open to feedback on how we want to represent these as requirements. |
||
| ## Commit message and pull request title format standards | ||
|
|
||
| > **Note**: Remove this section before opening the pull request | ||
|
|
@@ -63,6 +83,7 @@ Please specify a release date (and environment, if applicable) to guarantee time | |
| `<commit type>: [JIRA-ticket-number] - <description>` | ||
|
|
||
| **Commit Types:** | ||
|
|
||
| - `feat`: New feature for the user (not a part of the code, or ci, ...). | ||
| - `fix`: Bugfix for the user (not a fix to build something, ...). | ||
| - `change`: Modifying an existing visual UI instance. Such as a component or a feature. | ||
|
|
||
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.
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.
What's the reasoning behind using checkboxes for reproduction/verification steps?
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.
Requiring slightly more engagement on the Author's part to confirm they did, in fact, verify those things since all of them should apply to every PR opened. (This differs from the consideration bullets above, where some did not always apply, which was cause for potential confusion.) Any concerns about the checks, or were you thinking it would be best to aim for consistency?
Related tangent:
I'm not sure I like the checklist -> bulleted change above either. I think it will encourage more automatic skimming. The potential confusion mentioned above could be solved by adding a line like "Please check only those that apply." before the consideration checklist.
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.
Gotcha! I always looked at this more as a spot to put step-by-step instructions for reviewers to follow when verifying PR changes, but I see plenty of value in emphasizing that authors should also be doing their part to self-review and test their PRs too.
Edit: Also totally hear you about the checkbox/bullet change. I like the idea of saving ourselves some clicks, but if we think it might come at the cost of having devs overlook important points that might bite us later then I could understand going back to the way it was.
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.
Oh shoot, I misread this comment and thought it was referring to the checks in the section below where Connie commented. That's my bad.
But the answer is somewhat similar! In retro, we talked about liking when Authors provide checklists for their reviewers - it's easier to review, so we want to encourage that! In that same vein, Authors should definitely also be confirming their own changes.
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.
I'm going to merge this change and we'll give the bullets a try. Will be keeping an eye out for whether the single checkbox works in our favor and raise the issue if we should revert!