3 ms·

I would say everything in the codebase should be auto formatted to a consistent style. This reduces whitespace noise in pull requests. No room for egotistical d

I would say everything in the codebase should be auto formatted to a consistent style. This reduces whitespace noise in pull requests. No room for egotistical developers to take ownership of files in a shared codebase and waste time on petty formatting arguments. It keeps everything professional and tidy. When someone commits changes to an existing file, they auto format before committing. If existing parts of the file already contain weird non standard formatting this creates distracting whitespace noise in the pull request that could have been prevented. It's a waste of time to not use standard formatting.

erik_seaberg 3y ago
Layout is semantic, just as names and comments are. I don't want a tool to blindly stomp any of these when it lacks understanding how they benefit readers. The answer to dirty diffs is to not make them. Every change should be intentional, and a reviewer should agree that it's important. "Reformat this block I haphazardly edited" is fine, but don't make the change bigger for no reason.
- sumedh 3y ago
  > I don't want a tool to blindly stomp any of these when it lacks understanding how they benefit readers Some dev will have 1 newline after a method ends, another dev will have two newlines, some might have 3 etc. Should you as a code reviewer be wasting time looking for such issues and putting comments tell the other to fix such issues?
  - erik_seaberg 3y ago
    It’s not my job to make you write exactly what I would have. I don’t care about an extra blank line, and I expect a reviewer to accept that it doesn’t matter. I draw the line at layout that is misleading about the code (e.g., dangling else that doesn’t parse the way it’s indented).
    - sumedh 3y ago
      > I don’t care about an extra blank line..I expect a reviewer to accept that it doesn’t matter. You dont care about an extra blank line, what about 2 extra lines, what about three and so on. Do you see why there is a need for a consistent style?
      - erik_seaberg 3y ago
        If it gets to a dozen I’d grumble about wanting to fit more code on screen, but this just isn’t a problem we’ve ever had. I think the urge to enforce complete uniformity is bad for a collegial high-trust team.
        sumedh 3y ago
        > If it gets to a dozen I’d grumble about wanting to fit more code on screen exactly, which is why such rules should be part of the code, no need to trust if such conventions are followed if the rule will take care of it. You should spend your time doing some meaningful not wasting it on such issues.
        deleted 3y ago
        [deleted]
- Mike_12345 3y ago
  Yes layout is semantic and the automatic layout configuration should not be random nonsense. Configure a style that makes sense and apply it. I've never seen that code that needed to be weirdly formatted with its own rules separate from other files in the codebase in order to make sense. That just sounds like a developer being a prima donna with their code style, not something that genuinely improves understanding and collaboration.
  - erik_seaberg 3y ago
    I’m thinking of cases like System.arraycopy(src, srcPos, // from dest, destPos, len) // to where I want to show the parallel relationships between src/srcPos and dest/destPos. Blind tools don’t know to do this, they either emit one long line or waste seven lines. Tools also won’t know how to format a DSL well at all, which has already been a problem with Spark jobs in Scala.
    - Mike_12345 3y ago
      Meh. That is so obvious to anyone who would use that function. Just put it on one line. If your function has too many parameters then the code probably needs refactoring. Yes agreed about DSL in Scala. Scala is complicated. I was a Scala dev for many years. We tried to avoid the more academic and overly abstract designs with that language.

erik_seaberg 3y ago

Layout is semantic, just as names and comments are. I don't want a tool to blindly stomp any of these when it lacks understanding how they benefit readers. The answer to dirty diffs is to not make them. Every change should be intentional, and a reviewer should agree that it's important. "Reformat this block I haphazardly edited" is fine, but don't make the change bigger for no reason.

sumedh 3y ago
> I don't want a tool to blindly stomp any of these when it lacks understanding how they benefit readers Some dev will have 1 newline after a method ends, another dev will have two newlines, some might have 3 etc. Should you as a code reviewer be wasting time looking for such issues and putting comments tell the other to fix such issues?
- erik_seaberg 3y ago
  It’s not my job to make you write exactly what I would have. I don’t care about an extra blank line, and I expect a reviewer to accept that it doesn’t matter. I draw the line at layout that is misleading about the code (e.g., dangling else that doesn’t parse the way it’s indented).
  - sumedh 3y ago
    > I don’t care about an extra blank line..I expect a reviewer to accept that it doesn’t matter. You dont care about an extra blank line, what about 2 extra lines, what about three and so on. Do you see why there is a need for a consistent style?
    - erik_seaberg 3y ago
      If it gets to a dozen I’d grumble about wanting to fit more code on screen, but this just isn’t a problem we’ve ever had. I think the urge to enforce complete uniformity is bad for a collegial high-trust team.
      - sumedh 3y ago
        > If it gets to a dozen I’d grumble about wanting to fit more code on screen exactly, which is why such rules should be part of the code, no need to trust if such conventions are followed if the rule will take care of it. You should spend your time doing some meaningful not wasting it on such issues.
        deleted 3y ago
        [deleted]
Mike_12345 3y ago
Yes layout is semantic and the automatic layout configuration should not be random nonsense. Configure a style that makes sense and apply it. I've never seen that code that needed to be weirdly formatted with its own rules separate from other files in the codebase in order to make sense. That just sounds like a developer being a prima donna with their code style, not something that genuinely improves understanding and collaboration.
- erik_seaberg 3y ago
  I’m thinking of cases like System.arraycopy(src, srcPos, // from dest, destPos, len) // to where I want to show the parallel relationships between src/srcPos and dest/destPos. Blind tools don’t know to do this, they either emit one long line or waste seven lines. Tools also won’t know how to format a DSL well at all, which has already been a problem with Spark jobs in Scala.
  - Mike_12345 3y ago
    Meh. That is so obvious to anyone who would use that function. Just put it on one line. If your function has too many parameters then the code probably needs refactoring. Yes agreed about DSL in Scala. Scala is complicated. I was a Scala dev for many years. We tried to avoid the more academic and overly abstract designs with that language.