search

cmhughes / latexindent.pl

Perl script to add indentation (leading horizontal space) to LaTeX files. It can modify line breaks before, during and after code blocks; it can perform text wrapping and paragraph line break removal. It can also perform string-based and regex-based substitutions/replacements. The script is customisable through its YAML interface.
GNU General Public License v3.0
884 stars 84 forks source link

Fix pre-commit usage #354

Closed Holzhaus closed 2 years ago

Holzhaus commented 2 years ago

This fixes pre-commit hooks with are not working by default. See the individual commit messages for details.

github-actions[bot] commented 2 years ago

Thank you for your contribution! Please can you change this pull request so that it goes to the develop branch? Thank you

cmhughes commented 2 years ago

Thanks for this, and for your time in looking at this

The discussion surrounding backup files has been had quite a few times.

I won't ever have an option for overwriting a file without a backup, regardless of how well documented it is.

I also don't want the default of anything to be 'overwrite', I want the user to have to specify that so that they consciously make that choice.

cmhughes commented 2 years ago

Here's the documentation on customising pre commit

https://latexindentpl.readthedocs.io/en/latest/sec-appendices.html#lst-pre-commit-config-yaml-cpan

Holzhaus commented 2 years ago

The discussion surrounding backup files has been had quite a few times.

I won't ever have an option for overwriting a file without a backup, regardless of how well documented it is.

Of course this is your prerogative since it's your project. I can only urge you to reconsider, because this makes it virtually impossible to use latexindent.pl as a pre-commit hook.

Pretty much every single code formatter out there allows overwriting without backup, e.g. eslint --fix, prettier --write or clang_format. In the latter case, it's even the default behavior.

When using git, the repository already acts as backup, so writing superfluous *.bak files is distracting and just a waste of space and SSD write cycles. Yes, people may be able to shoot themselves in the foot if they are really dedicated to do that. You already acknowledged in https://github.com/cmhughes/latexindent.pl/issues/333#issuecomment-1022109624 that people take responsibility by configuring or using latexindex.pl in a certain way.

I also don't want the default of anything to be 'overwrite', I want the user to have to specify that so that they consciously make that choice.

The user conciously made that choice by running pre-commit install. It's more or less the sole purpose of it. Without overwriting, the pre-commit hook does not work at all. Output is printed to stdout but never seen by the user and the hook always passes, even if latexindent.pl would reformat it.

This is pretty bad, because now the user thinks that all files are properly formatted even though they aren't. This is a pretty major bug and should not be default behavior.

When using the pre-commit hook with overwriting enabled, all staged changes are reformatted. So although the files are overwritten, the staging area still acts as the backup. You can see the changes made by latexindent.pl (and the other pre-commit hooks) by using git diff. You can even selectively stage these changes for commit by using git add -p, or you can discard them with git checkout -p or even git checkout -- . This is not only safe but also way more convenient than working with *.bak files.

cmhughes commented 2 years ago

OK, many thanks. I'll be away from this for a few days but will return to this.

On Sat, 26 Mar 2022, 21:58 Jan Holthuis, @.***> wrote:

The discussion surrounding backup files has been had quite a few times.

I won't ever have an option for overwriting a file without a backup, regardless of how well documented it is.

Of course this is your prerogative since it's your project. I can only urge you to reconsider, because this makes it virtually impossible to use latexindent.pl as a pre-commit hook.

Pretty much every single code formatter out there allows this, e.g. eslint --fix https://eslint.org/docs/user-guide/command-line-interface#options, prettier --write https://prettier.io/docs/en/cli.html or clang_format https://clang.llvm.org/docs/ClangFormat.html. In the latter case, it's even the default behavior.

In the git case, the repository already acts as backup, so writing superfluous *.bak files is distracting and just a waste of space and SSD write cycles. Yes, people may be able to shoot themselves in the foot if they are really dedicated to do that. You already acknowledged in #333 (comment) https://github.com/cmhughes/latexindent.pl/issues/333#issuecomment-1022109624 that people take responsibility by configuring or using latexindex.pl in a certain way.

I also don't want the default of anything to be 'overwrite', I want the user to have to specify that so that they consciously make that choice.

The user conciously made that choice by running pre-commit install. It's more or less the sole purpose of it. Without overwriting, the pre-commit hook does not work at all. Output is printed to stdout but never seen by the user and the hook always passes, even if latexindent.pl would reformat it.

This is pretty bad, because now the user thinks that all files are properly formatted even though they aren't. This is a pretty major bug and should not be default behavior.

When using the pre-commit hook with overwriting enabled, all staged changes are reformatted. So although the files are overwritten, the staging area still acts as the backup. You can see the changes made by latexindent.pl (and the other pre-commit hooks) by using git diff. You can even selectively stage these changes for commit by using git add -p, or you can discard them with git checkout -p or even git checkout -- . This is not only safe but also way more convenient than working with *.bak files.

— Reply to this email directly, view it on GitHub https://github.com/cmhughes/latexindent.pl/pull/354#issuecomment-1079782365, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAQ7CYFP3H7NSGTTZCFT4N3VB6B7BANCNFSM5RXVX3YQ . You are receiving this because you commented.Message ID: @.***>

cmhughes commented 2 years ago

Thanks again for this.

summary

If you make the above changes, then documentation/sec-appendices.tex will need updating. (I can do this part, if you prefer.)

reasons for rejecting overwriteWithoutBackup

I have a responsibility to the community of users of latexindent.pl which includes a variety of different users.

  1. latexindent.pl connects implicitly into various external tools (for example atom-beautify, LaTeX-Workshop ) about which I have no control. So, let's say that the developers of such an external tool were to pick up a switch that says overwriteWithoutBackup and makes it the default for their application; and then a user picks up this external tool, and doesn't have a back-up system in place, and then something goes wrong. Who is responsible: the user? the developer of the external tool? me, as the author of latexindent.pl? Without a back-up, there's just too much risk.
  2. latexindent.pl is used explicitly by a number of users. Again, some users may pick up the tool, think that overwriteWithoutBackup is a good idea, something goes wrong, they don't have a version control system in place, and now everything has gone wrong. Who is responsible: the user? me, as the author of latexindent.pl?

In both scenarios, this is not a risk I am willing to take. The community of users use latexindent.pl on all kinds of different files, some of them are tremendously important (like a thesis); if something goes wrong, there must be a back-up in place, and I need to ensure that latexindent.pl does everything it can to make sure that this happens.

Pretty much every single code formatter out there allows overwriting without backup, e.g. eslint --fix, prettier --write or clang_format. In the latter case, it's even the default behavior.

That's interesting, but ultimately not relevant to me as I am not responsible for these tools.

things that users can do to influence back-up procedure

You, and other users, can use the following strategies to affect the back-up procedure:

  1. use onlyOneBackUp: 1 so that you only get one back-up file (see https://latexindentpl.readthedocs.io/en/latest/sec-default-user-local.html#backup-and-log-file-preferences)
  2. use the -wd switch so that you only activate the back-up procedure if the input text does not match the output text (see https://latexindentpl.readthedocs.io/en/latest/sec-how-to-use.html)
  3. use the -c switch to redirect your back-up files to another directory (see https://latexindentpl.readthedocs.io/en/latest/sec-how-to-use.html)
  4. use a local .gitignore so that your back-up files are ignored.
tdegeus commented 2 years ago

I understand your reasoning (though I think the license covers you here).

So it is probably futile, but still: There could be a pre-commit only wrapper of latexindent.pl that is not part of the public API. It would overwrite without backup. (This is probably not very nice to maintain).

Holzhaus commented 2 years ago
  • I'll happily accept changes to readme.md; the rev should have the current version, V3.17 (I have a system for updating this as I update version numbers, so it should be kept up to date during releases)

Ok. Just note that it's very common to just leave the rev: field on empty string and just run pre-commit autoupdate afterwards to pick the latest tag automatically.

  • if you change the pre-commit-hooks.yaml lines to args: ["--overwriteIfDifferent", "--silent"] that should be fine; I take your point about users of version control expecting this to be default. Users of version control and pre-commit should understand how to control their files, as you've detailed.

Sure, I can also use --overwriteIfDifferent. --local should also be used, because you usually want every contributor to use the same settings, and the local flags allows putting a .latexindent.yaml in the root of the git repository. Otherwise you can end up with conflicting reformattings by different contributors.

  1. latexindent.pl connects implicitly into various external tools (for example atom-beautify, LaTeX-Workshop ) about which I have no control. So, let's say that the developers of such an external tool were to pick up a switch that says overwriteWithoutBackup and makes it the default for their application; and then a user picks up this external tool, and doesn't have a back-up system in place, and then something goes wrong. Who is responsible: the user? the developer of the external tool? me, as the author of latexindent.pl? Without a back-up, there's just too much risk.

Either the user or the developers of the external tool, depending on the license used. Certainly not you. What if that external tool creates a temporary --cruft directory and immediately delete it after every invocation? Would you be responsible as well?

  1. latexindent.pl is used explicitly by a number of users. Again, some users may pick up the tool, think that overwriteWithoutBackup is a good idea, something goes wrong, they don't have a version control system in place, and now everything has gone wrong. Who is responsible: the user? me, as the author of latexindent.pl?

The user. There was a clear warning in the help text and the user chose to use the flag anyway. And this is my "common sense" interpretation, not a legal one - on that side side, you're safe anyway, because the license of latexindent.pl is pretty clear:

  1. Limitation of Liability.

IN NO EVENT UNLESS REQUIRED BY APPLICABLE LAW OR AGREED TO IN WRITING WILL ANY COPYRIGHT HOLDER, OR ANY OTHER PARTY WHO MODIFIES AND/OR CONVEYS THE PROGRAM AS PERMITTED ABOVE, BE LIABLE TO YOU FOR DAMAGES, INCLUDING ANY GENERAL, SPECIAL, INCIDENTAL OR CONSEQUENTIAL DAMAGES ARISING OUT OF THE USE OR INABILITY TO USE THE PROGRAM (INCLUDING BUT NOT LIMITED TO LOSS OF DATA OR DATA BEING RENDERED INACCURATE OR LOSSES SUSTAINED BY YOU OR THIRD PARTIES OR A FAILURE OF THE PROGRAM TO OPERATE WITH ANY OTHER PROGRAMS), EVEN IF SUCH HOLDER OR OTHER PARTY HAS BEEN ADVISED OF THE POSSIBILITY OF SUCH DAMAGES.

But yeah, you made clear that you don't want your users to be able to disable backups under any circumstance. However, if I have to delete these backups manually all the time and accidently rm the wrong file....aren't you at risk of being responsible as well? :sweat_smile:

In both scenarios, this is not a risk I am willing to take. The community of users use latexindent.pl on all kinds of different files, some of them are tremendously important (like a thesis); if something goes wrong, there must be a back-up in place, and I need to ensure that latexindent.pl does everything it can to make sure that this happens.

I'm using this for my thesis btw.

things that users can do to influence back-up procedure

You, and other users, can use the following strategies to affect the back-up procedure:

[...]

None of this makes backups go away, only hides them from gitignore. No worries, I can live with just using my own fork of this tool. I just thought would be nice to help other people out that have the same use case.

I'll update this PR with the changes you outlined above though.

cmhughes commented 2 years ago

That's great, thanks so much for your time on this.

However, if I have to delete these backups manually all the time and accidently rm the wrong file....aren't you at risk of being responsible as well?

I can't be responsible for every command every user runs :) But I am responsible for the behaviour of this latexindent.pl. I appreciate your understanding.

This will be part of the next release. It looks like I might be doing a minor release soon.

Thanks again!

cmhughes commented 2 years ago

Released at https://github.com/cmhughes/latexindent.pl/releases/tag/V3.17.1

Thanks again!