5 ms·

I personally believe that making diffs more human friendly is the next step of evolution we need. I work with a team on a NodeJS+React project and nearly every

I personally believe that making diffs more human friendly is the next step of evolution we need. I work with a team on a NodeJS+React project and nearly every other PR shows up as "Something changed in package.json, something changed in package-lock.json, some static assets added/modified, some JSON changed" etc and it makes reviewing code quite unwieldy (esp. since it forces folks to use Github UI interface to even see blob diffs. which is quite opinionated on when it collapses a file in the PR, and how it determines what changed in a file). I feel like git was perfect when "code" was nearly almost completely text and patches were sent over email, but there's a lot more boilerplate+blob data that goes into git today and git needs to evolve to support this. I feel that once tools like Difftastic [1] and similar get more mainstream, and ideally more firmly entrenched within git itself, it will make code reviewing much smoother process rather than having to depend on Github or any other proprietary service. [1] https://github.com/Wilfred/difftastic https://github.com/Wilfred/difftastic

alexchamberlain 4y ago
I feel like you may be blaming the tool for a social problem there. We have a simple rule: no changes to package* in a feature PR. Adding a dependency? Cool, upgrade everything first, then add it fresh. (Be reasonable there- the idea is that the add should be clean, so if that can be done without doing a major upgrade, that is of course fine.)
- bmitc 4y ago
  It sounds like you're just working around the limitations of the tools. Edit: I think it's possible that I misunderstood, given the downvotes. I thought they meant that a dependency should be added in one PR, and then the feature needing that dependency in a separate PR. What I think they actually meant is that if adding a dependency requires upgrades of other dependencies, then upgrade the existing dependencies in one PR and then add the new dependency and feature in another single PR. That seems to make sense but not be necessarily a hard rule. As I said below though, I still think the tooling in this space is terrible. Even for dependencies, I don't want line and text changes in config and lock files. I want something that summarizes what dependencies were added or upgraded.
  - alexchamberlain 4y ago
    Simply separating concerns so that each PR/commit focuses on 1 thing.
    - bmitc 4y ago
      But what is the one thing if you're just adding a dependency, where you're only adding it so that you can subsequently add a feature using it in a separate PR? The atomic action is adding the feature which requires the new dependency. Unless I misunderstood what you wrote. I still agree with the original comment in that diffing technology feels decades behind, and that a lot of what we do as software engineers are working around things the tooling should be doing better on.
      - LtWorf 4y ago
        You can review pr commit by commit… if the commits are poorly made just reject the changes and ask to make them properly.
      - sokoloff 4y ago
        It seems no different than adding a feature dark in one PR and turning it on in a second. “The atomic action is launching the new feature to users” seems like an equivalent argument (and one with which I disagree).
      - alexchamberlain 4y ago
        Sorry, I don't think I explained myself well. Generally when you see a lot of package* noise, it's because you've upgraded your dependencies, added one and made a feature change all at once. The main thing that needs to be separated is the upgrading part.
  - sshine 4y ago
    Or a limitation of the human mind. If you jam too many unrelated changes into one diff, people stop paying attention, because now reviewing it requires many minutes and note-taking. Since hundreds of line changes can have little or no effect, but a single line change somewhere else can have drastic effects, it is essential to separate the unimportant from the important. Changes in dependencies is a not unimportant change, but they tend to get treated like noise because those changes are associated with automated tooling, like the package tool. Updating dependencies separately solves a social problem, not a technical one.
WorldMaker 4y ago
Semantic diffs are great (I did some experiments with it myself [0]), but we can't entirely automate away "narrative problems" in a PR. No matter how good a diff is, a diff tells you what changed but not why and sometimes not even how. That's what we need good commit messages for. That's what we need good PR descriptions to do. If a branch has a ton of seemingly unrelated files modified (automated tool output or not), sometimes you have to ask the narrative questions: "Why did this change? What does it have to do with the other changes in this branch?" Admittedly, narrative problems are hard to solve in general. It's a lot easier to "build a new tool" than to train a junior developer to always explain why/how in commit messages, to order their commits to "tell a story" of what the branch accomplishes, to avoid unrelated changes in digressions and asides along the way (moving those into their own branches/narratives), to tell that story in a PR description in a way that is useful to understanding the whys/hows of the branch (especially if the commits themselves lack some of the narrative or aren't ordered well for proper storytelling). I often settle for one or two of those at a time from a given junior developer. I don't think we can automate ourselves out of narrative problems. I think narrative problems are one of the unsung creative problems of our industry and something that separates good programmers from great programmers. It's a human skill that takes practice. > I feel like git was perfect when "code" was nearly almost completely text and patches were sent over email That "perfect" never existed. Even codebases like the Linux kernel that work entirely in email still have their random binary blobs (often from outside vendors) and auto-generated files from tools. What the email flow focuses, arguably to some (such as many of the folks at Sourcehut) better than most other PR tools, is that conversation around the narrative and they why/hows. You expect a lengthy narrative discussion in a mailing list. Sometimes people see those comment fields in a Github PR and expect then to be less about narrative and more about nitpicking specific lines than having narrative discussions. (Nitpicking happens in mailing list discussions, too. It's mostly unavoidable. But to nitpick on a mailing list you have to do a lot more copying and pasting by hand.) There is something of a different story telling "pressure" when you are staring at an empty email with a file auto-attached than when you click the PR button in Github and get pages full of diffs and all the commit logs laid out before you before you ever start writing. (Sometimes people do see that and think their job is "done" and what they would write in the PR description would be redundant, because it is not a "blank email" that needs a greetings and salutation and maybe a description of the attachment.) [0] https://github.com/WorldMaker/tokdiff https://github.com/WorldMaker/tokdiff

alexchamberlain 4y ago

I feel like you may be blaming the tool for a social problem there. We have a simple rule: no changes to package* in a feature PR. Adding a dependency? Cool, upgrade everything first, then add it fresh. (Be reasonable there- the idea is that the add should be clean, so if that can be done without doing a major upgrade, that is of course fine.)

bmitc 4y ago
It sounds like you're just working around the limitations of the tools. Edit: I think it's possible that I misunderstood, given the downvotes. I thought they meant that a dependency should be added in one PR, and then the feature needing that dependency in a separate PR. What I think they actually meant is that if adding a dependency requires upgrades of other dependencies, then upgrade the existing dependencies in one PR and then add the new dependency and feature in another single PR. That seems to make sense but not be necessarily a hard rule. As I said below though, I still think the tooling in this space is terrible. Even for dependencies, I don't want line and text changes in config and lock files. I want something that summarizes what dependencies were added or upgraded.
- alexchamberlain 4y ago
  Simply separating concerns so that each PR/commit focuses on 1 thing.
  - bmitc 4y ago
    But what is the one thing if you're just adding a dependency, where you're only adding it so that you can subsequently add a feature using it in a separate PR? The atomic action is adding the feature which requires the new dependency. Unless I misunderstood what you wrote. I still agree with the original comment in that diffing technology feels decades behind, and that a lot of what we do as software engineers are working around things the tooling should be doing better on.
    - LtWorf 4y ago
      You can review pr commit by commit… if the commits are poorly made just reject the changes and ask to make them properly.
    - sokoloff 4y ago
      It seems no different than adding a feature dark in one PR and turning it on in a second. “The atomic action is launching the new feature to users” seems like an equivalent argument (and one with which I disagree).
    - alexchamberlain 4y ago
      Sorry, I don't think I explained myself well. Generally when you see a lot of package* noise, it's because you've upgraded your dependencies, added one and made a feature change all at once. The main thing that needs to be separated is the upgrading part.
- sshine 4y ago
  Or a limitation of the human mind. If you jam too many unrelated changes into one diff, people stop paying attention, because now reviewing it requires many minutes and note-taking. Since hundreds of line changes can have little or no effect, but a single line change somewhere else can have drastic effects, it is essential to separate the unimportant from the important. Changes in dependencies is a not unimportant change, but they tend to get treated like noise because those changes are associated with automated tooling, like the package tool. Updating dependencies separately solves a social problem, not a technical one.

WorldMaker 4y ago

Semantic diffs are great (I did some experiments with it myself [0]), but we can't entirely automate away "narrative problems" in a PR. No matter how good a diff is, a diff tells you what changed but not why and sometimes not even how. That's what we need good commit messages for. That's what we need good PR descriptions to do. If a branch has a ton of seemingly unrelated files modified (automated tool output or not), sometimes you have to ask the narrative questions: "Why did this change? What does it have to do with the other changes in this branch?" Admittedly, narrative problems are hard to solve in general. It's a lot easier to "build a new tool" than to train a junior developer to always explain why/how in commit messages, to order their commits to "tell a story" of what the branch accomplishes, to avoid unrelated changes in digressions and asides along the way (moving those into their own branches/narratives), to tell that story in a PR description in a way that is useful to understanding the whys/hows of the branch (especially if the commits themselves lack some of the narrative or aren't ordered well for proper storytelling). I often settle for one or two of those at a time from a given junior developer. I don't think we can automate ourselves out of narrative problems. I think narrative problems are one of the unsung creative problems of our industry and something that separates good programmers from great programmers. It's a human skill that takes practice. > I feel like git was perfect when "code" was nearly almost completely text and patches were sent over email That "perfect" never existed. Even codebases like the Linux kernel that work entirely in email still have their random binary blobs (often from outside vendors) and auto-generated files from tools. What the email flow focuses, arguably to some (such as many of the folks at Sourcehut) better than most other PR tools, is that conversation around the narrative and they why/hows. You expect a lengthy narrative discussion in a mailing list. Sometimes people see those comment fields in a Github PR and expect then to be less about narrative and more about nitpicking specific lines than having narrative discussions. (Nitpicking happens in mailing list discussions, too. It's mostly unavoidable. But to nitpick on a mailing list you have to do a lot more copying and pasting by hand.) There is something of a different story telling "pressure" when you are staring at an empty email with a file auto-attached than when you click the PR button in Github and get pages full of diffs and all the commit logs laid out before you before you ever start writing. (Sometimes people do see that and think their job is "done" and what they would write in the PR description would be redundant, because it is not a "blank email" that needs a greetings and salutation and maybe a description of the attachment.) [0] https://github.com/WorldMaker/tokdiff https://github.com/WorldMaker/tokdiff