mirrors-autopep8 usage (vs autopep8-wrapper) touches files that were not changed

[suntzu86]

I'm running pre-commit run --all-files

In the old way:

repos:
 -   repo: git@git.yelpcorp.com:mirrors/pre-commit/pre-commit-hooks
     rev: v1.4.0
     hooks:
     -   id: autopep8-wrapper
     ...other hooks...

if autopep8 has no changes for a file, then that file was not modified. So autopep8 passing meant no files are modified.

In the new way:

repos:
 -   repo: git@git.yelpcorp.com:mirrors/pre-commit/pre-commit-hooks
     rev: v1.4.0
     ...other hooks...
-    repo: https://github.com/pre-commit/mirrors-autopep8
     rev: v1.4.2
     hooks:
     -   id: autopep8

if autopep8 has no changes, it still modifies the file. Here, autopep8 passing modifies every file.

By modify, I mean that the time column of ls -l changes. And in emacs, it will complain that files have changed on disk.

Also, in the old way, autopep8 printed out the modified files: Files were modified by this hook. Additional output: (and then a list of files separated by newline).
In the new way, it does not--just says Files were modified by this hook.

[asottile]

Yeah there's probably going to be slight differences but autopep8-wrapper was dropped from this repo because it wasn't really maintainable (and it conflicted with flake8 #328 #319 (comment)). It also had subtle differences to upstream (as you're noticing!) and so users were often surprised by these behaviour differences (among those: it handled newlines differently, it forced UTF-8 encoding, ...).

The fortunate thing though, is now that mirrors-autopep8 is just a stupid-simple repo that directly calls autopep8 -- you can propose your issues upstream! I think it's completely reasonable that it doesn't modify a file if it doesn't need to modify a file and I'm sure they would take a PR to fix that.

The "printing the filename" bit is kind of unfortunate, I wish upstream autopep8 printed something, maybe that's proposable too. The old code was only printing because pre-commit-hooks forced it to print: https://github.com/pre-commit/pre-commit-hooks/pull/321/files#diff-58bc70c007e2f111699c5851bc1dac05L21

The historical context for why autopep8-wrapper existed at all was due to an old behaviour of pre-commit: it didn't know anything about modifications. This was fixed way back in 0.6.3 (2015-11-12) and so this wrapper had kinda been unnecessary ever since.

So I guess in conclusion, there isn't really anything actionable here -- other than suggesting some changes directly to autopep8

[asottile]

Anyway, thanks for the issue 🎉 -- since there's nothing actionable from the pre-commit-hooks side, I'm going to close this.

[suntzu86]

Thanks for the quick response + thorough context!
That makes sense... looking at your code link it's very clear why autopep8-wrapper did not touch unchanged files.

I'll poke autopep8 and see what they're willing to do on both no touching unchanged files and logging some basic info on what changed.