5 ms·

Great list, but I'd argue that point 6 is more often indicative of tooling failure. Why is it hard to review something that changes both formatting and the code

• pimlottc 6y ago
  I agree tooling should be improved, but the change should be legible in more than one tool. Chances are good that someone will end up looking at the diff in the future in their favorite IDE, on the command line, or in who knows what else tool. The simplest and clearest solution is to split out the changes in separate commits.
• piotrkaminski 6y ago
  Better tooling is definitely feasible, but confounding formatting and semantic changes also messes up blame for the affected lines and often makes a line's history misleading.
  • dpcx 6y ago
    The problem I've found with this statement is that then no one ever changes formatting for fear of "messing up blame for the affected lines," and then no reformatting ever gets done. At some point, take the leap and do it (assuming you're not completely changing the formatting of the entire file/project/what-have-you).
  • jsnell 6y ago
    Blame is also a tool. If a reasonable change messes up blame, that's a tooling problem :) For example git-blame can ignore whitespace, detect code movement inside a file, and even code movement across files. There's no reason why that has to be the limit of how smart blame can be.
• mtlynch 6y ago
  It depends how aggressively your reformatter changes your code. For example, Prettier will take this JavaScript line: document.addEventListener('keydown', e => { console.log(e) }); and convert it to this, which changes both whitespace and syntax: document.addEventListener("keydown", (e) => { console.log(e); }); I don't think there are any diff tools that can hide the differences there, even though they're superficial. [0] https://prettier.io/ https://prettier.io/
  • jlund-molfese 6y ago
    You could try https://github.com/josephfrazier/prettier-diff https://github.com/josephfrazier/prettier-diff , but it's inconvenient to ask members of your team to download an entirely separate diff tool
    • mtlynch 6y ago
      Oh, that's interesting. I didn't realize that existed. Thanks for the pointer! Personally, I wouldn't want a niche diff tool globally wrapping my diff views. I'd worry too much about missing something important. And I agree that it's also a tough sell to get your teammates to all use this one particular diff tool.
• jedimastert 6y ago
  This can apply beyond a white space change, but rather larger non-functional changes as well. Something like moving a function up a layer to make it available elsewhere before using it elsewhere. It this point, the "automatic detection tool" is whether or not the current tests fail without having to change them.
• toast0 6y ago
  > Why is it hard to review something that changes both formatting and the code? The opposite problem is also a tooling problem: Why is it hard to save your changes to code, but not the formatting.
• zmmmmm 6y ago
  Unnecessary refactoring and whitespace changes are hard fails for my code reviews. There are rare exceptions where refactoring is essentially intertwined with the change being performed, but they are rare. I need to be able to see the intent and minimal definition of a change. Anything that obscures that is obnoxious not just to the reviewer but everyone after who might have to merge or understand the change. Perhaps it is because I manage code that is "high risk" and hence "I can't understand your change" equates to "it can't go into production" makes a difference. Apart from that, it also ruins the chance to cherry pick / separately merge parts of the change. To me this is pure laziness and selfishness and lack of discipline on the developers part.