7 ms·

> Avoid bike-shedding about style/formatting/etc. If this is important where you work, automate with tools; I'd love some suggestions for how to get people to

> Avoid bike-shedding about style/formatting/etc. If this is important where you work, automate with tools; I'd love some suggestions for how to get people to avoid this type of bike-shedding. While I'd love for us to use some tools to automate this, we have a huge codebase with existing code that doesn't follow the rules, and it's not in my ability to implement said tools. (Big company, lots of overhead, will affect a lot of people, don't have a lot of time, not my department, etc.) One of the things I hate is having a different comment for every single place where I misplaced a star or ampersand. My natural tendency is to do it one way, but our style guidelines prefer it the other way, so it's something I miss frequently. Having 30 comments in a review that are all "move the star to the other side" over and over again just makes me hate the reviewer. (Particularly if they don't provide any useful feedback.) A single comment on the first one that says something like, "This doesn't match the style guidelines, and I see several others that don't match. Please fix them all," would be highly preferable. And if, god forbid, I miss one, just get over it. It can be fixed later. It's not nearly as important as having correct functionality in our app, and I'm pretty busy!

isbvhodnvemrwvn 4y ago
Are you sure it's not in your ability? Whenever I introduced something like that people were in general indifferent-to-happy, as long as you did the majority of the job and they could somewhat easily run those tools (e.g. anything written in Java is more or less a lost cause because people cling to their IDEs)
physicsguy 4y ago
Tools are easy. You add it to the CI/CD pipeline, it fails if it doesn’t meet the guideline, people have to run the tool to get the merge in. On first run it’s painful, so if you’re not starting a project, use an autoformatter so it’s not all manual work. If you really want, you can even make your CI pipeline push commits with autofixes Of course if you don’t have a CI pipeline that’s easy to integrate steps like this, you’ve got major issues to deal with elsewhere…
- thewebcount 4y ago
  I don't have the permissions to add it to the CI/CD pipeline. I've filed bugs on our CI/CD to add it, but it's apparently not a high priority (you know, until someone sits down to review code, then suddenly it's the most important thing in the world).
  - physicsguy 4y ago
    If I were you, I’d be looking to move on from your company then. Developers should own the testing part of the pipeline at the very least.
0xffff2 4y ago
Sorry, but formatting is on you. If you don't like the comments, don't commit inconsistent code. Set up a local auto-formatter with the right rules. If enough of the existing code you're working on doesn't conform, set the formatter to only run manually and get in the habit of running it before committing.
- thewebcount 4y ago
  Yeah, I've been looking for something to do that, but haven't found something that fits into my workflow yet.
woojoo666 4y ago
Set up the linters to only lint code that was changed. This way legacy code doesn't need to be fixed all at once, but will slowly get fixed over time
- dserodio 4y ago
  Yes, please. It's awful when a one-line PR changes the whole file to fix formatting
mason55 4y ago
Everyone is talking about how to get the style guide integrated into your CI. But it sounds to me like your concern is that other reviewers frequently point out your style guide violations. You could set up some local lint rules just for yourself. You'd need to set it up to only run against code that you changed. And if you don't have an automated style guide you'd have to come up with your own. But it seems like once you get over the hump you could just slowly add rules as you get tired of forgetting about them/people pointing them out to you. > And if, god forbid, I miss one, just get over it. It can be fixed later. It's not nearly as important as having correct functionality in our app, and I'm pretty busy! This is a concerning attitude. If the team has agreed on a set of style guides to follow then you should follow them, argue to change them, or move on. If you don't have time to adhere to the style guide now, why do you think you will have more time to go back and fix it later?
- hackernewds 4y ago
  In addendum, if a couple of style guidelines don't apply to a couple of lines of code for a couple of people, would it have any meaning at all?
- HWR_14 4y ago
  Their concern isn't that other reviewers point it out. It's that they think they shouldn't have to follow the guidelines: > My natural tendency is to do it one way, but our style guidelines prefer it the other way Which is fine if it's a reason it's an occasionally slip up. But apparently, in spite of knowing this tendency, they frequently submit code to review where they've messed up to such a degree that reading every instance is considered annoying. Think of the poor reviewer who had to put all the comments in!
  - thewebcount 4y ago
    Well that's a very uncharitable way to read what I wrote. I previously worked on code bases where it had to be one way. It became pretty engrained in me. Now it needs to be the other way. I try my best to use the "new" style now, but because I'm human and busy, I sometimes forget. I spend most of my time focusing on the problem I'm working on and not how I'm placing characters on the line. I'm not the only one, so I don't think I'm being a particularly bad programmer. We have one reviewer who's really concerned about star placement. It's sometimes the only thing he points out, and he points it out on a lot of different people's work, so I'm certainly not worse than anyone else on the team. As I say, I do try, but sometimes I forget. The other thing is sometimes I didn't write the code in question. I hit the issue where I'm refactoring and I move a chunk of previously-written code that didn't follow the guidelines, but I didn't modify the code I moved, so now it looks like I "forgot" but in reality, I just moved some code around. How much time do we want to spend reformatting code instead of working on the actual problem we're trying to fix or implement? I would love if we could automate this, but see my other comment about the issues with that. I think these are all understandable things that busy people have to deal with. It's not that I don't think I shouldn't have to follow the rules. It's that some rules are much more important than others, and differences in code that have the same readability but don't in any way change the functionality seem less important to me than things that can actually cause bugs.
NateEag 4y ago
I haven't yet taken this concept to completion, but when I had a similar issue years ago I hacked out a cheesy program to filter out static analysis messages on lines not modified by the diff: https://github.com/NateEag/diff-check https://github.com/NateEag/diff-check Maybe someone's built the complete version out there somewhere?
sverhagen 4y ago
>(Particularly if they don't provide any useful feedback.) If my eyes stumble on formatting violations every other line, you're probably also not going to get good feedback from me. Code formatting isn't very important, yet it is.
mcv 4y ago
I've been on a project where we dynamically generated tons of different Cypher queries. To keep those queries readable, consistent formatting was very important, and I had to keep bringing that up in code review. I hated it. It absolutely should be automated, but I could not find any tools for formatting Cypher query strings inside Javascript code. I'd probably have to write it myself.
DaSpood 4y ago
Something as simple as a pre-commit hook that checks files that were changed in your commit and rejects the commit if the rules aren't followed is a very cheap and effective way to enforce a coding style. For a python project for example, you could set pre-commit to check formatting, linting and typing, which means that when your code reaches the remote repository, before any CI is even started or any PR is openned (before you even push really), you know your code follows the rules that have been set for the project. Thus everyone knows you don't need to spend time in the code review looking for these mistakes and instead focus on the actual features. pre-commit does not even need to be pushed to remote to work, so if your company has no plan to enforce it but it still helps you, you can still use it, unlike a CI. As for this: > And if, god forbid, I miss one, just get over it. It can be fixed later. It's not nearly as important as having correct functionality in our app, and I'm pretty busy! It's true that one mistake is easy to fix later, but it's just as easy to fix now. If you postpone it once you will postpone it again when it's not highlighted in a PR diff, it's just a recipe for never fixing it. Every language has automatic linters/formaters that check (and even fix) your code according to rules you define, turning the work of finding these mistakes into a simple `lint ./src/` command. It's takes a small amount of time to setup but saves you a lot of time down the line.
- thewebcount 4y ago
  I love the idea of a pre-commit hook. I did actually implement this for a past project, and the rest of the team hated it. Whenever they thought it was being "unfair" to them, I'd get email about it because I wrote it. The problem with my current project is that it has millions of lines of code and that code all has different styles. So even if you only check the lines that actually changed in a pre-commit hook, you can end up with different styles in the same file, which can be as bad as using the wrong style. At least if it's consistently wrong, working within a single file is doable. > It's true that one mistake is easy to fix later, but it's just as easy to fix now. Eh... maybe. I see your point, but I guess what I'm saying is "choose your battles." Every minor thing one points out like this takes time away from me fixing bigger issues, so be judicious in how much of a stickler you want to be for the rules. Yep, I can fix it now pretty easily, and then I won't be working on the next bug until it's fixed. And the way we have GitHub set up, if I make any change, I have to get at least 1-3 reviewers to re-review the code and re-approve it. That's the real time waster because everyone else if fixing the nit-picky things for their latest pull request. If they take too long, then I have merge conflicts and I have to address those before I can commit, and, oops! that causes the review clock to start over. We get to do it all again! Talk about Kafka-esque. If we were starting a new project, I would probably demand that we start with some sort of pre-commit formatter that forces the appropriate style onto our code, but we're unlikely to be doing that for the foreseeable future, unfortunately.

isbvhodnvemrwvn 4y ago

Are you sure it's not in your ability? Whenever I introduced something like that people were in general indifferent-to-happy, as long as you did the majority of the job and they could somewhat easily run those tools (e.g. anything written in Java is more or less a lost cause because people cling to their IDEs)

physicsguy 4y ago

Tools are easy. You add it to the CI/CD pipeline, it fails if it doesn’t meet the guideline, people have to run the tool to get the merge in. On first run it’s painful, so if you’re not starting a project, use an autoformatter so it’s not all manual work. If you really want, you can even make your CI pipeline push commits with autofixes Of course if you don’t have a CI pipeline that’s easy to integrate steps like this, you’ve got major issues to deal with elsewhere…

thewebcount 4y ago
I don't have the permissions to add it to the CI/CD pipeline. I've filed bugs on our CI/CD to add it, but it's apparently not a high priority (you know, until someone sits down to review code, then suddenly it's the most important thing in the world).
- physicsguy 4y ago
  If I were you, I’d be looking to move on from your company then. Developers should own the testing part of the pipeline at the very least.

0xffff2 4y ago

Sorry, but formatting is on you. If you don't like the comments, don't commit inconsistent code. Set up a local auto-formatter with the right rules. If enough of the existing code you're working on doesn't conform, set the formatter to only run manually and get in the habit of running it before committing.

thewebcount 4y ago
Yeah, I've been looking for something to do that, but haven't found something that fits into my workflow yet.

woojoo666 4y ago

Set up the linters to only lint code that was changed. This way legacy code doesn't need to be fixed all at once, but will slowly get fixed over time

dserodio 4y ago
Yes, please. It's awful when a one-line PR changes the whole file to fix formatting

mason55 4y ago

Everyone is talking about how to get the style guide integrated into your CI. But it sounds to me like your concern is that other reviewers frequently point out your style guide violations. You could set up some local lint rules just for yourself. You'd need to set it up to only run against code that you changed. And if you don't have an automated style guide you'd have to come up with your own. But it seems like once you get over the hump you could just slowly add rules as you get tired of forgetting about them/people pointing them out to you. > And if, god forbid, I miss one, just get over it. It can be fixed later. It's not nearly as important as having correct functionality in our app, and I'm pretty busy! This is a concerning attitude. If the team has agreed on a set of style guides to follow then you should follow them, argue to change them, or move on. If you don't have time to adhere to the style guide now, why do you think you will have more time to go back and fix it later?

hackernewds 4y ago
In addendum, if a couple of style guidelines don't apply to a couple of lines of code for a couple of people, would it have any meaning at all?
HWR_14 4y ago
Their concern isn't that other reviewers point it out. It's that they think they shouldn't have to follow the guidelines: > My natural tendency is to do it one way, but our style guidelines prefer it the other way Which is fine if it's a reason it's an occasionally slip up. But apparently, in spite of knowing this tendency, they frequently submit code to review where they've messed up to such a degree that reading every instance is considered annoying. Think of the poor reviewer who had to put all the comments in!
- thewebcount 4y ago
  Well that's a very uncharitable way to read what I wrote. I previously worked on code bases where it had to be one way. It became pretty engrained in me. Now it needs to be the other way. I try my best to use the "new" style now, but because I'm human and busy, I sometimes forget. I spend most of my time focusing on the problem I'm working on and not how I'm placing characters on the line. I'm not the only one, so I don't think I'm being a particularly bad programmer. We have one reviewer who's really concerned about star placement. It's sometimes the only thing he points out, and he points it out on a lot of different people's work, so I'm certainly not worse than anyone else on the team. As I say, I do try, but sometimes I forget. The other thing is sometimes I didn't write the code in question. I hit the issue where I'm refactoring and I move a chunk of previously-written code that didn't follow the guidelines, but I didn't modify the code I moved, so now it looks like I "forgot" but in reality, I just moved some code around. How much time do we want to spend reformatting code instead of working on the actual problem we're trying to fix or implement? I would love if we could automate this, but see my other comment about the issues with that. I think these are all understandable things that busy people have to deal with. It's not that I don't think I shouldn't have to follow the rules. It's that some rules are much more important than others, and differences in code that have the same readability but don't in any way change the functionality seem less important to me than things that can actually cause bugs.

NateEag 4y ago

I haven't yet taken this concept to completion, but when I had a similar issue years ago I hacked out a cheesy program to filter out static analysis messages on lines not modified by the diff: https://github.com/NateEag/diff-check https://github.com/NateEag/diff-check Maybe someone's built the complete version out there somewhere?

sverhagen 4y ago

>(Particularly if they don't provide any useful feedback.) If my eyes stumble on formatting violations every other line, you're probably also not going to get good feedback from me. Code formatting isn't very important, yet it is.

mcv 4y ago

I've been on a project where we dynamically generated tons of different Cypher queries. To keep those queries readable, consistent formatting was very important, and I had to keep bringing that up in code review. I hated it. It absolutely should be automated, but I could not find any tools for formatting Cypher query strings inside Javascript code. I'd probably have to write it myself.

DaSpood 4y ago

Something as simple as a pre-commit hook that checks files that were changed in your commit and rejects the commit if the rules aren't followed is a very cheap and effective way to enforce a coding style. For a python project for example, you could set pre-commit to check formatting, linting and typing, which means that when your code reaches the remote repository, before any CI is even started or any PR is openned (before you even push really), you know your code follows the rules that have been set for the project. Thus everyone knows you don't need to spend time in the code review looking for these mistakes and instead focus on the actual features. pre-commit does not even need to be pushed to remote to work, so if your company has no plan to enforce it but it still helps you, you can still use it, unlike a CI. As for this: > And if, god forbid, I miss one, just get over it. It can be fixed later. It's not nearly as important as having correct functionality in our app, and I'm pretty busy! It's true that one mistake is easy to fix later, but it's just as easy to fix now. If you postpone it once you will postpone it again when it's not highlighted in a PR diff, it's just a recipe for never fixing it. Every language has automatic linters/formaters that check (and even fix) your code according to rules you define, turning the work of finding these mistakes into a simple `lint ./src/` command. It's takes a small amount of time to setup but saves you a lot of time down the line.

thewebcount 4y ago
I love the idea of a pre-commit hook. I did actually implement this for a past project, and the rest of the team hated it. Whenever they thought it was being "unfair" to them, I'd get email about it because I wrote it. The problem with my current project is that it has millions of lines of code and that code all has different styles. So even if you only check the lines that actually changed in a pre-commit hook, you can end up with different styles in the same file, which can be as bad as using the wrong style. At least if it's consistently wrong, working within a single file is doable. > It's true that one mistake is easy to fix later, but it's just as easy to fix now. Eh... maybe. I see your point, but I guess what I'm saying is "choose your battles." Every minor thing one points out like this takes time away from me fixing bigger issues, so be judicious in how much of a stickler you want to be for the rules. Yep, I can fix it now pretty easily, and then I won't be working on the next bug until it's fixed. And the way we have GitHub set up, if I make any change, I have to get at least 1-3 reviewers to re-review the code and re-approve it. That's the real time waster because everyone else if fixing the nit-picky things for their latest pull request. If they take too long, then I have merge conflicts and I have to address those before I can commit, and, oops! that causes the review clock to start over. We get to do it all again! Talk about Kafka-esque. If we were starting a new project, I would probably demand that we start with some sort of pre-commit formatter that forces the appropriate style onto our code, but we're unlikely to be doing that for the foreseeable future, unfortunately.