3 ms·

The reality is that most of your comments will be reminding people to follow the style guide or to not organize their code like monkeys with type writers. If y

The reality is that most of your comments will be reminding people to follow the style guide or to not organize their code like monkeys with type writers. If you're leaving comments about style on PRs then your tooling isn't doing a good enough job. You should have a linter and a formatter configured, everyone should be sharing the same config, and they should always be run on a prepush hook (or earlier... run them on file saves if possible). Badly formatted code should make the the push fail so the dev has to fix the problem before they can open a PR. Force pushes and skipping hooks should be vigorously discouraged. That way badly formatted code shouldn't make it to the repo and your PRs can focus on more important things. Obviously this is really hard to implement on a project that's old and has thousands of linter errors. If you didn't set it up on a greenfield project right at the start you can't really complain too much though.

DangitBobby 4y ago
> Badly formatted code should make the the push fail so the dev has to fix the problem before they can open a PR I'd rather people check in their WIP than have it sitting on their machine. Let GH actions worry about whether the code is formatted correctly.
gombosg 4y ago
I think that often it's not just coding style (yes, a linter should catch indentation errors), but coding conventions/requirements to enforce an idiomatic code structure in the code base. Linters can't catch that. E.g. how to organize files, where to put business logic, how tests are structured, which libraries to use, how to handle non-blocking or concurrent code, errors, code documentation etc.
allset_ 4y ago
Style is much more than formatting. It's best practices around naming, when and how to add comments, and how to write clear and maintainable code.
- onion2k 4y ago
  I don't think I'd call any of those things "style". Those things are the code itself, not the style it's written in. They're definitely things that PR reviewers should be looking at.

DangitBobby 4y ago

> Badly formatted code should make the the push fail so the dev has to fix the problem before they can open a PR I'd rather people check in their WIP than have it sitting on their machine. Let GH actions worry about whether the code is formatted correctly.

gombosg 4y ago

I think that often it's not just coding style (yes, a linter should catch indentation errors), but coding conventions/requirements to enforce an idiomatic code structure in the code base. Linters can't catch that. E.g. how to organize files, where to put business logic, how tests are structured, which libraries to use, how to handle non-blocking or concurrent code, errors, code documentation etc.

allset_ 4y ago

Style is much more than formatting. It's best practices around naming, when and how to add comments, and how to write clear and maintainable code.

onion2k 4y ago
I don't think I'd call any of those things "style". Those things are the code itself, not the style it's written in. They're definitely things that PR reviewers should be looking at.