Hacker News Reader

Comment by PhilipRoman | Hacker News Reader

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.

View on HN

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:)

Is that new? I don’t remember that feature, working with gitlab a few months back.

> Is that new?

It's neither new nor novel. Even GitHub allows anyone to post a commit on a feature branch, regardless of who created it.

It's not a GitLab or GitHub feature. It's a Git feature: the ability to post a commit to a branch.

create alias dont-fuck-with-my-branch

git reset --hard HEAD~1

git push -f

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.

I was just joking but surely I can revert a commit on a feature branch, no?

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.

sorry for misunderstanding, I am NOT trying to hide the fact I reverted, hence the alias dont-fuck-with-my-branch :) and I was overall just joking…

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.

https://github.blog/news-insights/product-news/suggested-cha...

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")

https://docs.gitlab.com/ee/user/project/merge_requests/revie...

https://docs.gitlab.com/ee/user/project/merge_requests/appro... and https://docs.gitlab.com/ee/user/project/merge_requests/appro...

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.

> Ping-pong with comments like "add empty line" is a waste of time for everyone.

And any manager that lets it happen without severe reprimands to the time-waster isn't worth working for.

We've all got way more important things to be spending our time on.

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

have the reviewer just adjust it themselves

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.

Sounds like a team of overvalued prima donnas.

Oh, so you've met software engineers? :)

IMO, the best way is to have automatic formatter shared by team.

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.

There's probaly a ton of projects that don't even have version control in 2024, let alone a linter.

Reply on news.ycombinator.com