3 ms·

I'm just thinking about this time at a previous job, I was reviewing a PR and they decided to just find/replace every variable and switch from snake to camel ca

by ge96 2y ago

I'm just thinking about this time at a previous job, I was reviewing a PR and they decided to just find/replace every variable and switch from snake to camel case. I was like "why are you guys doing this, not part of the job". There was some back and forward on that. This is a place where PRs weren't about reviews but just a process to follow, ask someone to approve/not expect feedback. edit: job = ticket task

teeray 2y ago
For stuff like this, the best thing is to have a style guide. If you disagree with something in the style guide (tabs/spaces, casing, etc.), you make a PR to the style guide and hash it out there. Also, those PRs need updates everywhere to make the codebase agree with the change and updates to relevant linters enforce it. Outside of that, the style guide is law.
dboreham 2y ago
It's also literally not part of the job.
Cthulhu_ 2y ago
What was the established code style (...if any) in that project? Anyway it doesn't sound like that was a very mature project or developers, not when the reviewer decide to just edit code instead of provide a review.
- ge96 2y ago
  the old/existing code was all underscore, they wanted to use camelcase instead. it's a dumb thing to be argue about I know but it made the code review harder when instead of 10s of line diffs there's almost a hundred granted easy to see just changing casing
  - willseth 2y ago
    I just insist that style only changes go in a separate commit.
    - JTyQZSnP3cQGa8B 2y ago
      And when it impacts a lot of files, it can break the compiler or introduce bugs. It MUST go in its own PR/MR.
bandrami 2y ago
This. There's consistency and there's yak-shaving. We had a guy who spent two months changing tabs to multiple spaces (or vice versa; whichever was in fashion at the time) mostly to get those sweet commits for "productivity" metrics. Yes, our bad for not realizing that sooner, but at some point you have to let inconsistencies go.

by ge96 2y ago

teeray 2y ago

For stuff like this, the best thing is to have a style guide. If you disagree with something in the style guide (tabs/spaces, casing, etc.), you make a PR to the style guide and hash it out there. Also, those PRs need updates everywhere to make the codebase agree with the change and updates to relevant linters enforce it. Outside of that, the style guide is law.

dboreham 2y ago

It's also literally not part of the job.

Cthulhu_ 2y ago

What was the established code style (...if any) in that project? Anyway it doesn't sound like that was a very mature project or developers, not when the reviewer decide to just edit code instead of provide a review.

ge96 2y ago
the old/existing code was all underscore, they wanted to use camelcase instead. it's a dumb thing to be argue about I know but it made the code review harder when instead of 10s of line diffs there's almost a hundred granted easy to see just changing casing
- willseth 2y ago
  I just insist that style only changes go in a separate commit.
  - JTyQZSnP3cQGa8B 2y ago
    And when it impacts a lot of files, it can break the compiler or introduce bugs. It MUST go in its own PR/MR.

bandrami 2y ago

This. There's consistency and there's yak-shaving. We had a guy who spent two months changing tabs to multiple spaces (or vice versa; whichever was in fashion at the time) mostly to get those sweet commits for "productivity" metrics. Yes, our bad for not realizing that sooner, but at some point you have to let inconsistencies go.