IMO the best way to handle stylistic choices (if at all) is to have the reviewer just adjust it themselves. Ping-pong with comments like "add empty line" is a waste of time for everyone.
Not just stylistic choices - I absolutely love the way ex. gitlab lets you do a suggested change in a comment, precisely because it lets me do all the work and the MR owner just has to click to accept if they agree. That lowers the bar for what I'm willing to bring up in the first place (though I personally still don't like to comment on matters of style or taste) while making it less work for everyone, which makes it much easier to give useful suggestions:)
I'm not sure about gitlab, but on github this is a distinct feature where you post a comment wrapped in three backticks and the language as suggestion. It'll format it as a diff that the author can one-click create a commit from.
Services like GitHub and GitLab support preventing changes that overwrite a repo's history, in addition to tracking history even when you force update a branch.
You can revert and even force-update the branch to rewrite it's history, but GitHub still tracks the old commits and even lists in the PR the events that rewrote the branch history.
I like GitLab's feature of "suggested change" whereby the submitter can offer the change as a one-click "accept change" but importantly the change is still under the original author, because in any sane(?) review process, authors are not able to approve their own merge request. In your workflow if the reviewer checked out the branch, did the needful, committed, and then resumed reviewing they'd have locked themselves out of approving (err, assuming the aforementioned "authors cannot approve their own merges")
I like this idea, it has the bonus of revealing how much the reviewer actually cares. I bet half the things they wouldn’t take the time to fix themselves.
> I like this idea, it has the bonus of revealing how much the reviewer actually cares.
And now you contributed to turn your team culture into shit, with a toxic mix of PR creators purposely shitting on the team's style guide and PR reviewers who have to constantly post follow-up commits to your work because you can't even put together and acceptable PR or clean up after yourself.
What if anyone who posts a PR addresses feedback anyone posts on their work and addresses them until you get the necessary and sufficient approvals?
> IMO the best way to handle stylistic choices (if at all) is to have the reviewer just adjust it themselves.
How does that inform the PR author that they failed to pay attention to a relevant detail, and more importantly contribute to not make it again?
If there is no feedback, problems will persist.
Nit comments add noise and failures to comply with a style guide is always a distraction on a PR, but that does not mean style guide violations should be ignored. It means that your team must work their way into making them a non-issue.
This means establishing an objective and usable set of rules and guidelines, and enforce them programmatically with a linter, not turning a blind eye to style violations.
> And any manager that lets it happen without severe reprimands to the time-waster isn't worth working for.
That would be something only a terribly incompetent manager would ever do.
A manager who is not terribly incompetent would either not take sides until explicitly asked to weigh in, and if and only if he was dragged into the discussion then the manager's only acceptable input would be to eliminate the problem for good by adopting a linter.
Everything else is a mistake and a complete waste of time.
Passive-aggressive minefield. Someone will see a change done to THEIR code, without the changer even asking, and it will feel like the person who did it is a passive aggressive dickhead. Resentment will brew, tempers will be lost. It will only get worse from there. Software engineers already aren't exactly known for their humbleness and ability to swallow their ego.
Isn’t that a given in 2024? I find it amazing that it still seems to be an issue in some places. Using a linter/formatter is a no-brainer at this point, and it’s been for years.
