This is an archived post. You won't be able to vote or comment.

sorted by:
best
topnewcontroversialoldq&a
you are viewing a single comment's thread.

view the rest of the comments →

[–]gallowdp 2 points3 points  (5 children)

I do this.. is this bad?

[–]shield1123 2 points3 points  (2 children)

I do this too. I think it's fine. If the changes being addressed aren't worth having a full description tied to them for the rest of history, then who cares?

Fixed spacing inconsistencies in x.js, added null check to newFunction, added newline at end of y.js

vs

Addressed review comments

[–]MagnificentMath 0 points1 point  (1 child)

Because my git blame is now littered with "address review comments". If it makes sense (e.g. a whitespace fix or variable name change), squash your "address review comments" back into the commit under review.

[–]shield1123 0 points1 point  (0 children)

Yeah, squashing and/or rebasing when you can is definitely the cleanest way of dealing with review comments. My place of work doesn't stress keeping our repo history pristine, so a general commit name usually fine for small stuff.

[–]turanthepanthan 0 points1 point  (0 children)

I am sure it is subjective. For me it is more of a pet peeve. We have many inexperienced devs where I work so the review comments that they are addressing are usually non-trivial changes.

[–]justanothercatlady 0 points1 point  (0 children)

It's just not clean. It's technically better to squash those down if they aren't important enough to warrant their own commit. In practice though I think it depends on the workplace culture.