Pull request template enhancements #5
Conversation
|
@MGibson1 , I remember a while back you had to remove the checkboxes we used to have like this in our PR templates; do you remember why we had to remove them? |
|
@differsthecat It's because the PR will show |
Ooh, that's a good point. I do want that for the actual checklist too independent of the change type. Do we feel like we get value out of that at all? Could go with a table and ✔️ or something if we do, but I don't think the code block is the answer. |
I do like the checkboxes, especially for the checklist section :p A table for the |
|
@withinfocus How will we extend this for different repos? For example, we have a check in clients about making sure all UI additions follow our accessibility guidelines that we don't need in this base one. |
|
About the 🚧 Type of change< !-- Pick one and remove the others --> 🐞 Bug fix So if we choose the second one, the PR would just stay as: 🚧 Type of change🚀 New feature development 📔 Objective... |
Simple copy and paste. This is our (new to many) template repo, so it's just a starting point. We want what matters to everyone here to get it started, then customize on each "installation". |
|
@fedemkr I think that could work too, as long as it's easy enough to cut what you need out (which seems like it is). |
I was thinking that it was pretty noisy in that section. Maybe even moving it into a dropdown? Example. |
|
Made a few small changes per the comments to review. I tried out the non-checklist for change types and thought it felt pretty good. I also added a (hidden) comment about the optionality of the checklist vs. rewording each item as I think that will get repetitive. |
.github/PULL_REQUEST_TEMPLATE.md
Outdated
| ## 🦮 Reviewer guidelines | ||
|
|
||
| - 👍 (`:+1:`) or similar for great changes | ||
| - 📝 (`:memo:`) or ℹ️ (`:information_source:`) for notes or general info | ||
| - ❓ (`:question:`) for questions | ||
| - 🤔 (`:thinking:`) or 💭 (`:thought_balloon:`) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion | ||
| - ❌ (`:x:`) or ⚠️ (`:warning:`) for problems or concerns needing attention | ||
| - 🌱 (`:seedling:`) or ♻️ (`:recycle:`) for future improvements or indications of technical debt | ||
| - ⛏ (`:pick:`) for minor or nitpick changes |
There was a problem hiding this comment.
I've been using conventional comments ever since I started at Bitwarden and I think that has been very appreciated. These emojis are very similar so I like the suggestion! Though I probably like conventional comments more 😅
For inspirarion, one specific part of CC that I think has been well received is the modifier non-blocking to a comment (ex: suggestion(non-blocking): make this variable private) which indicates that it's "ok to skip but consider if you have the time"
There was a problem hiding this comment.
I really enjoy conventional commits and we could even lint on enforcing that but ... that's a big request and with squashing may not even be useful. I hadn't heard of conventional comments before but yes this is going for the same thing but with the power of a visual expression; lots of articles about this being a great way to deliver feedback.
Specific to your "suggestion" I could see 📝 or 🌱 being used.
There was a problem hiding this comment.
Hmm, which category would you say the following example belongs to?
function greet(flag: boolean): string {
let toReturn;
if (flag === true) {
toReturn = 'hello';
} else if (flag === false) {
toReturn = 'world';
} else if (flag == null) {
throw new Error();
}
return toReturn;
}suggestion: this could be rewritten using guard statements to improve readability. We could also return the values directly instead of having to keep track of an extra variable. See example:
function greet(flag: boolean): string {
if (flag == null) {
throw new Error();
}
if (flag) {
return "hello";
}
return "world";
}In conventional comments terms I would probably not assign this an issue since it's still fully functional code, albeit with poor readability. And sometimes these things end up being personal preferences instead of objective improvements. I use suggestion when I don't expect but am prepared for legitimate push back. I wouldn't use nitpick since those I would drop at the first push back.
According to the categories in this PR I'm not sure which I would choose. I don't think this is nitpicking ⛏️ , but it's not really a showstopper ❌
I think I would end up using
It feels hard to explain in words since it feels like it comes down to nuances between words and emojis. Am I making sense here?
There was a problem hiding this comment.
Totally making sense, and I will keep thinking about this one too. Some other sources I found for emoji usage:
There was a problem hiding this comment.
@coroiu after a little more thought I just pushed a change that incorporates one of the above sites' suggestion on 🎨 for what you're describing. Lemme know what you think.
There was a problem hiding this comment.
I think that's a great solution! Thanks for taking the time to decipher what felt like over-thinking on my part 😅
|
For anyone subscribed and reviewing, feel free to commit to this branch or suggest alternatives, even if we revert! |
|
I like the Reminders section, thanks @withinfocus ! |
.github/PULL_REQUEST_TEMPLATE.md
Outdated
| ## 🦮 Reviewer guidelines | ||
|
|
||
| - 👍 (`:+1:`) or similar for great changes | ||
| - 📝 (`:memo:`) or ℹ️ (`:information_source:`) for notes or general info | ||
| - ❓ (`:question:`) for questions | ||
| - 🤔 (`:thinking:`) or 💭 (`:thought_balloon:`) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion | ||
| - ❌ (`:x:`) or ⚠️ (`:warning:`) for problems or concerns needing attention | ||
| - 🌱 (`:seedling:`) or ♻️ (`:recycle:`) for future improvements or indications of technical debt | ||
| - ⛏ (`:pick:`) for minor or nitpick changes |
There was a problem hiding this comment.
I think that's a great solution! Thanks for taking the time to decipher what felt like over-thinking on my part 😅
|
@fedemkr @differsthecat @eliykat @mimartin12 @justindbaur any final feelings on this? |
1f87c4f
|
@fedemkr I'll let you give the final 👍! |
🎟️ Tracking
Internal development.
🚧 Type of change
📔 Objective
Adds some fun to the template and expands on what info we should collect and where, namely:
📋 Code changes
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:x:) or:warning:) for problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes