4 ms·

Feature request: Add a flag that allows you to specify which lines to format. I'm an auto-formatting true believer, but I hate doing code reviews where the aut

Feature request: Add a flag that allows you to specify which lines to format. I'm an auto-formatting true believer, but I hate doing code reviews where the author has run a formatting tool that introduced diffs all over parts of the file that they didn't actually touch. It makes it really hard to review the actual change, and you have to worry about missing something. With something like ClangFormat's '--lines' flag, you can configure the tool to only modify the lines that the author actually changed. This removes most of the pain that comes with adopting an auto-formatting tool, and your codebase will gradually converge to the fully auto-formatted state over time. This also makes it easier for to introduce improvements to the formatting tool itself, because the improvements won't immediately force a bunch of diffs on code that was already "blessed" by the tool.

weberc2 7y ago
Sounds like you're using it wrong. The autoformatter is for the whole repo, not for individual contributors. The point is that the whole repo has the same style, so you should only ever get diffs on the lines that were changed or the first time you run the autoformatter on the repo (and then you shouldn't be making manual changes that will be hidden amid the autoformatter noise).
- alexhutcheson 7y ago
  Most larger orgs/projects aren't going to be willing to reformat all the code in their repo in one big bang. Regardless of what promises the tool claims to make, you need to be concerned about behavior changes or breakages, and most projects don't have high enough test coverage to cover everything. Depending on the size of your repo, it might not even be technically possible to make this change all at once. It's also much harder to get organizational buy-in on "let's change all existing code in one swoop" vs. "let's require all new code changes to be formatted using this tool". As I mentioned above, formatting only the diffs also removes most of the pain of any changes to the formatting tool itself that aren't diff-stable.
  - jhrmnn 7y ago
    Black verifies that the abstract syntax trees of the original and reformatted code are identical. There is practically zero chance of breaking changes.
  - weberc2 7y ago
    I shouldn’t have said repo, but rather some agreed upon unit of code. It could be a single package or file as long as all contributors touching that file adhere. You should not use black for any unit of code that you can’t get buy-in on. Black is a technical tool; it doesn’t solve political problems.
Mawr 7y ago
>[...] I hate doing code reviews where the author has run a formatting tool that introduced diffs all over parts of the file that they didn't actually touch. [...] Teach them to commit properly: 1. Run an autoformatter on the file 2. Commit 3. Make desired changes 4. Commit again The same principle applies to refactoring as well, you want to keep your commits 'atomic' aka containing a single unit of work. Autoformatting and changing the code are two separate units.

weberc2 7y ago

Sounds like you're using it wrong. The autoformatter is for the whole repo, not for individual contributors. The point is that the whole repo has the same style, so you should only ever get diffs on the lines that were changed or the first time you run the autoformatter on the repo (and then you shouldn't be making manual changes that will be hidden amid the autoformatter noise).

alexhutcheson 7y ago
Most larger orgs/projects aren't going to be willing to reformat all the code in their repo in one big bang. Regardless of what promises the tool claims to make, you need to be concerned about behavior changes or breakages, and most projects don't have high enough test coverage to cover everything. Depending on the size of your repo, it might not even be technically possible to make this change all at once. It's also much harder to get organizational buy-in on "let's change all existing code in one swoop" vs. "let's require all new code changes to be formatted using this tool". As I mentioned above, formatting only the diffs also removes most of the pain of any changes to the formatting tool itself that aren't diff-stable.
- jhrmnn 7y ago
  Black verifies that the abstract syntax trees of the original and reformatted code are identical. There is practically zero chance of breaking changes.
- weberc2 7y ago
  I shouldn’t have said repo, but rather some agreed upon unit of code. It could be a single package or file as long as all contributors touching that file adhere. You should not use black for any unit of code that you can’t get buy-in on. Black is a technical tool; it doesn’t solve political problems.

Mawr 7y ago

>[...] I hate doing code reviews where the author has run a formatting tool that introduced diffs all over parts of the file that they didn't actually touch. [...] Teach them to commit properly: 1. Run an autoformatter on the file 2. Commit 3. Make desired changes 4. Commit again The same principle applies to refactoring as well, you want to keep your commits 'atomic' aka containing a single unit of work. Autoformatting and changing the code are two separate units.