Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pull request template enhancements #5

Merged
merged 8 commits into from
Mar 29, 2023
Merged

Pull request template enhancements #5

Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
dce7bc7
Pull request template enhancements
withinfocus Mar 17, 2023
25f0d65
List of change types for selection
withinfocus Mar 18, 2023
ceadf7b
Wording for checks
withinfocus Mar 18, 2023
9963c5f
Make change types a list for when there are multiple applicable
withinfocus Mar 18, 2023
7d7492e
Move away from checklist and back to review reminders, now expanded
withinfocus Mar 22, 2023
0dcc62e
Comment for indicating reviewer guidelines are optional
withinfocus Mar 23, 2023
3c802c7
Add suggestions and clarify the more severe warnings
withinfocus Mar 23, 2023
1f87c4f
Mention fixed width images
withinfocus Mar 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 40 additions & 19 deletions .github/PULL_REQUEST_TEMPLATE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,28 +1,49 @@
## Type of change
## 🎟️ Tracking

<!-- (mark with an `X`) -->
<!-- Paste the link to the Jira or GitHub issue or otherwise describe / point to where this change is coming from. -->

```
- [ ] Bug fix
- [ ] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other
```
## 🚧 Type of change

## Objective
<!-- Choose those applicable and remove the others. -->

<!--Describe what the purpose of this PR is. For example: what bug you're fixing or what new feature you're adding-->
- 🐛 Bug fix
- 🚀 New feature development
- 🧹 Tech debt (refactoring, code cleanup, dependency upgrades, etc.)
- 🤖 Build/deploy pipeline (DevOps)
- 🎂 Other

## Code changes
## 📔 Objective

<!--Explain the changes you've made to each file or major component. This should help the reviewer understand your changes-->
<!--Also refer to any related changes or PRs in other repositories-->
<!-- Describe what the purpose of this PR is, for example what bug you're fixing or new feature you're adding. -->

- **file.ext:** Description of what was changed and why
## 📋 Code changes

## Before you submit
<!-- Explain the changes you've made to each file or major component. This should help the reviewer understand your changes. -->
<!-- Also refer to any related changes or PRs in other repositories. -->

- Please add **unit tests** where it makes sense to do so (encouraged but not required)
- If this change requires a **documentation update** - notify the documentation team
- If this change has particular **deployment requirements** - notify the DevOps team
- **file.ext:** Description of what was changed and why.

## 📸 Screenshots

<!-- Required for any UI changes; delete if not applicable. -->

## ✅ Checklist

<!-- Some checks are optional -- select what you've done when it applies. -->

- [ ] Contributor guidelines followed
- [ ] Communicated to DevOps any deployment requirements
- [ ] Ran all formatters and local linters
- [ ] Written new unit and / or integration tests where applicable
- [ ] Updated any necessary documentation or informed the documentation team
- [ ] Noted any required translations

## 🦮 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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@coroiu coroiu Mar 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ❌ ⚠️ either (these emojis also give me the feeling that the author has done something really wrong, which I wouldn't say is the case here). I'd also rather we fix it now and not add this to future debt, so I wouldn't really say it's a 🌱 or ♻️ either.

I think I would end up using ⚠️ , but I am trying to make a friendly suggestion which ⚠️ doesn't really feel like.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally making sense, and I will keep thinking about this one too. Some other sources I found for emoji usage:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's a great solution! Thanks for taking the time to decipher what felt like over-thinking on my part 😅