Removal of 👌 for code review changes

[smoliji]

Hello @carloscuesta 😎!

• Emoji: 👌
• Code: :ok_hand:
• Description: Updating code due to code review changes.

I'd like to propose a removal of the emoji, added in #93.

Reason: It is a bad practice to describe a VCS change as a code review change. Code reviews exist to reveal bugs, bad namings, usages, code performance etc. No change from a code review is made for no reason.

The flaws are there, they were just pointed out by the reviewer and they need to be fixed/refactored/optimized/.... And it is that, which should be in the commit message. With appropriate emoji in our case.

[smolijar]

I agree. Before incorporating any breaking changes, I think we should resolve #317 and start using semver.

[codemacabre]

I vote for removing the :ok_hand:, but only in terms of the emoji itself, not the concept of commits referring to code review changes. The emoji (and its physical hand gesture counterpart) has been corrupted by its misuse and appropriation as a symbol of hate (see https://selfdefined.netlify.com/#ok-hand).

Instead, I propose:

• 🦉 :owl: (my personal preference)
• 👁 :eye: or 👀 :eyes:
• 👁‍🗨 :eye_speech_bubble: (potentially problematic due to its origin)

I would argue that a dedicated gitmoji for code review changes is important, as there's value in referring back to commits that were specifically changed due to a discussion with other developers. While these changes will have root reasons for being identified and changed, it should be the choice of author of the commit (or the development team) whether to identify the change as the root category (refactoring, optimising, etc.), or as a code review change.

[smolijar]

1. I feel like the correctness argument as as valid as removing 👽 not to offend people with extreme opinions on existence of the aliens. The severity might be different (might not), but removing it because the symbol had an abuse of minor impact on the semantics (I bet more people see OK, thank WP. In contrast Swastika, where impact was far greater) is unwise. What if in similar fashion ✨ was used. Would we change that and break compatibly of the most common symbol?

it should be the choice of author of the commit (or the development team) whether to identify the change as the root category (refactoring, optimizing, etc.)

I agree, and that's why we should not throw all CR related changes into one category. After CR you can fix typos, optimize, fix bugs, add features, update tests. Why should these changes belong to a different category just because it was suggested in CR? To give credit to the author? If that's the case, use e.g. Reported-by trailer from official standard.

:ok_hand: IMO provides harmful semantic overlap, just like #208.

[elliot-nelson]

Re: CR Feedback as not a valid concept -- would you put multiple emojis on a single commit in that case?

I'm sure this varies team-to-team, but typically a post-PR "CR Feedback" commit is a nice catchall for "ah, thanks all, I fixed a couple var names, deleted that unnecessary line, fixed a comment, and added one unit test, please re-review". In general, most of my teammates would not prefer to have 4 different post-PR commits for one round of review change, even if it's addressing different issues.

[smoliji]

Re: CR Feedback as not a valid concept -- would you put multiple emojis on a single commit in that case?

Of course not! I would put as many commits as it is needed for the CR changes. Each having a single emoji for the change the commit does.

Main purpose of the project history is to answer for each commit a question "What does this change do?", not "Why does this change?" (even though this is also a good info, but that's where issue references come in).

most of my teammates would not prefer to have 4 different post-PR commits for one round of review change, even if it's addressing different issues.

Why do you treat the post-PR commits differently from pre-PR commits? They all make changes to the same code base, they form the same commit history.

If I'd once allow commits such as CR Feedback, history could also look like this, and it would be perfectly fine by the given example:

(BAD)

• CR Feedback
• Issue 8
• Issue 7 cont., issue 6
• Issue 7

But I want to know, by looking at the commit history what do the commits change. I don't really care if the colleague sitting next to the developer advised it, or it was the reviewer of the PR.

(GOOD)

• Fix linting
• Add unit test for cat
• Log each command
• Support multiple args in cat
• Add cat command

[elliot-nelson]

Ah, I see - it's just a different use case then.

Most likely in the long term, the whole PR is going to be squashed into a single one on merge anyway, so the individual commits are temporary.

So the primary purpose in the short term is for the returning reviewers, in which case, I personally do prefer to see the former example -- "CR fixes", then other commits if other changes were made.

[smolijar]

Most likely in the long term, the whole PR is going to be squashed into a single one on merge anyway, so the individual commits are temporary.

A daresay this is not a common practice. When working on non-trivial projects, history development of feature branches is important.

[wouterds]

I usually append it with another emoji when it's been pointed out in code review.

E.g. when a bug is pointed out in a code review I just commit it as follows

👌🐛 Code should return here instead of continuing

[smolijar]

From the history perspective, as @smoliji stated the information that the changes comes from a CR is not relevant.

As for using multiple emojis,

Only one symbol should be used per commit since this helps to organize and split commits better kind of “atomic commits”.

I think it's a good sign that 👌 is redundant, or overshadowing different, more important semantics.

[johannchopin]

I also agree to remove this gitmoji since it not define what has been changed but how. What do you think about this issue @carloscuesta, @vhoyer?

[vhoyer]

Definitely think we should remove this. Personally I don't use it, due to those reasons, and I don't like seeing people using it (not that I say anything about it 😅), as I don't think it's not descriptive enough.

[vhoyer]

We should bundle this and the #435 all in the same breaking-change release

[carloscuesta]

It’s a yes for me on removing this gitmoji!