Support diffing files instead of modifying

[michaelquinn32]

I am really excited by the prospect of a styler pre-commit hook, like the example using scalafmt here:
https://medium.com/zyseme-technology/code-formatting-scalafmt-and-the-git-pre-commit-hook-3de71d099514

To do that, there needs to be a way to use styler that supports diffing instead of modifying in place; we wouldn't want the latter during a precommit. Is it possible to write the styled files to a tmp directory? If so, you could combine readLines and the diffobj package to show results during a precommit.
https://cran.r-project.org/web/packages/diffobj/vignettes/diffobj.html

[lorenzwalthert]

Thanks for sharing the article, I linked it in #467, where we are already working on the pre-commit hook. I am not sure I understand why we need diffing for the pre commit hook when implemented as part of https://pre-commit.com. Could you elaborate on that a bit?
If you don't want to modify in place, you could always read and write the contents manually and transform it with styler::style_text().

[michaelquinn32]

Mostly, it's a workflow consideration.

Right now, I

1. commit everything
2. run styler
3. go through a diff tool (usually Rstudio's) to check the changes
4. accepting or revert where needed.
5. Commit again.

I don't like modifying a user's files during a pre-commit, but I would instead like,

1. run pre-commit
2. Get style diffs as suggestions for additional changes
3. Add or remove as necessary
4. Commit.

The second process seems more transparent and is more in line with approaches that other tools use (at least in my experience).

[michaelquinn32]

Black, a python formatter, supports the first workflow that I mentioned before. Instead of letting you see what it will do, it forces you to go back a review changes without much suggestion, adding more steps.
https://ljvmiranda921.github.io/notebook/2018/06/21/precommits-using-black-and-flake8/

[michaelquinn32]

Yapf's precommit also changes the files and then tells you to go back too. So... maybe what I would like is less common.
https://github.com/google/yapf/blob/master/plugins/pre-commit.sh

[lorenzwalthert]

I see. I also would not want the pre-commit hook to style my files (without showing me the diff) and then commit the combined changes right away. Just not sure how to implement the first approach. Cc: @katrinleinweber.

[katrinleinweber]

Thanks for developing my idea further :-) In this particular case however, I'm afraid we disagree: a formatter can absolve one from worrying about how the code is formatted. Accepting that the git show diff is different from the git add diff is a logical consequence, IMHO.

[lorenzwalthert]

Accepting that the git show diff is different from the git add diff is a logical consequence, IMHO.

I don't think I get that :-) So you think it's ok for the hook to modify our staged changes and then commit right away?

[katrinleinweber]

If the hook is configured to do that (like running styler), then of course its OK. I think I don't get why one would see that as a problem :-)

What may be weird initially: lines which hadn't been styled before have larger diffs after a commit (styling + content changes). I get this worry. But for all lines that have been styled at least once, the diffs will become smaller next time and only contain content changes again. Isn't that the point of auto-formatting code?

[lorenzwalthert]

So as of the current implementation in #467 and I think this is generally the case with pre-commit.com, when the hook modifies a file, changes are not committed right away, but the new changes are simply not staged and you have to stage the file again, and run git commit so maybe I misunderstood the process. But yes this is anyways not what @michaelquinn32 wanted but I think for styler, the current procedure is ok and I am not sure I understand why this creates extra steps @michaelquinn32? At least when using the RStudio git tab, one can simply revert the changes introduced by the running the pre-commit.

[katrinleinweber]

Oops, my bad! My self-made hook explicitly enforces the behaviour I described above:

for f in $(git diff --name-only --cached | grep '.R')
do
	Rscript -e "styler::style_file(file.path('$f'))"
	git add $f
done

What you describe is indeed the default behaviour. If I remove my git add $f, I see it as well. Apologies for introducing some confusion here :-/ I had configured this hook about a year ago.

[lorenzwalthert]

I think we can close this now given the implementation in #467.