XS✔ ◾ Do you know how to indicate mandatory vs suggested PR changes? (#…

…9493) * Added rule * Auto-fix Markdown files * Update rules/mandatory-vs-suggested-pr-changes/rule.md Co-authored-by: Matt Goldman [SSW] <[email protected]> * Update rules/mandatory-vs-suggested-pr-changes/rule.md Co-authored-by: Matt Goldman [SSW] <[email protected]> * Update rules/mandatory-vs-suggested-pr-changes/rule.md Co-authored-by: Matt Goldman [SSW] <[email protected]> * Update rules/mandatory-vs-suggested-pr-changes/rule.md Co-authored-by: Matt Goldman [SSW] <[email protected]> * PR comments * Update --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Matt Goldman [SSW] <[email protected]>
SSWConsulting · Oct 28, 2024 · 3185608 · 3185608
1 parent 4c540fe
commit 3185608
Show file tree

Hide file tree

Showing 2 changed files with 62 additions and 0 deletions.
diff --git a/categories/software-engineering/rules-to-better-pull-requests.md b/categories/software-engineering/rules-to-better-pull-requests.md
@@ -24,6 +24,7 @@ index:
 - page-owner
 - clean-git-history
 - pr-suggest-changes
+- mandatory-vs-suggested-pr-changes
 ---
 
 Pull Requests are the backbone of an effective development team. That's why it's crucial to ensure that everyone on the team understands the expectations around Pull Requests.
diff --git a/rules/mandatory-vs-suggested-pr-changes/rule.md b/rules/mandatory-vs-suggested-pr-changes/rule.md
@@ -0,0 +1,61 @@
+---
+seoDescription: Master effective pull request reviews by categorizing feedback into ‘Change,’ ‘Consider,’ and ‘PBI’ for clear, actionable guidance. Improve collaboration and streamline approvals with structured, prioritized comments
+type: rule
+title: Do you know how to indicate mandatory vs suggested PR changes?
+uri: mandatory-vs-suggested-pr-changes
+authors:
+  - title: Daniel Mackay
+    url: https://www.ssw.com.au/people/daniel-mackay
+  - title: Matt Goldman
+    url: https://www.ssw.com.au/people/goldie
+related:
+  - todo-tasks
+created: 2024-10-28T17:00:00.000Z
+guid: 6F5AE6FC-F29A-4938-B012-FDE7F669998C
+---
+
+When reviewing another developer's pull request, review comments can fall into a mix of categories. Some changes might be required, while others might be optional. Do you know the best way to tell the requester which is which?
+
+<!--endintro-->
+
+## Types of Pull Request Feedback
+
+When reviewing pull requests, comments can often be divided into two categories:
+
+* **Change** - you must change this if you want me to approve
+* **Consider** – consider this suggestion, but don’t feel obliged to change (i.e. I would do this, but I won’t block you on it)
+* **PBI** – Important, but create a PBI to fix this in future
+
+By prefixing the comment with one of the categories above, the reviewer can make it clear to the author if they must make the change or not.
+
+When adding 'PBI' comments, a "good Samaritan" approach is to create the PBI and link to it (add a [TODO task](https://www.ssw.com.au/rules/todo-tasks/)). Alternatively if you just leave the comment and want the requester to do it, they should do the same (add a TODO with a link to the PBI) before you approve it.
+
+## Pull Request Comment Examples
+
+::: greybox  
+This could be refactored to be better
+:::  
+::: bad  
+Figure: Bad example - not clear if the author should make the change or not
+:::
+
+::: greybox  
+Change – check for null reference and throw if found
+:::  
+::: good  
+Figure: Good example - clear that the change should be made before the PR will be approved
+:::
+
+::: greybox  
+Consider – break long method into several smaller ones
+:::  
+::: good  
+Figure: Good example - the reviewer has left a suggestion, but it's up to the author do change or not
+:::
+
+::: greybox  
+PBI – Consolidate these xUnit ‘Fact’ tests to use ‘InlineData’
+:::  
+::: good  
+Figure: Good example - This change doesn't need to be done now, but should be added to the backlog with a comment in the code
+:::