Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Print actual files being fixed by hooks #681

Closed
bagerard opened this issue Jan 9, 2018 · 5 comments
Closed

Print actual files being fixed by hooks #681

bagerard opened this issue Jan 9, 2018 · 5 comments

Comments

@bagerard
Copy link
Contributor

bagerard commented Jan 9, 2018

Hi,

First of all thanks for pre-commit, it's awesome!
While running isort as a hook, I noticed that it wasn't printing the files being fixed which isn't handy IMO. I could do a PR in isort but I thought it could be integrated in pre-commit. In fact, the current behaviour is that it prints "Files were modified by this hook. Additional output:" whenever a hook modifies a file, but it does not print the actual files.

I believe it could be useful to print the files being changed and not relying on the hook to do so. I think I can do that by parsing the output of "git diff --no-ext-diff" and extract filenames. The only issue I see is that it will be redundant with hooks that are printing that already (like autopep8-wrapper). Another option would be to print the modified files only if the hook returned empty stdout/stderr.

Let me know what you think, I'd be happy to work on this. If you don't like it, no problem I'll just do a PR in isort. Thanks!

@bagerard bagerard changed the title Printing actual files being fixed by hooks Print actual files being fixed by hooks Jan 9, 2018
@asottile
Copy link
Member

asottile commented Jan 9, 2018

One option that might help today (without any changes) -- there's a --show-diff-on-failure option to pre-commit run. One example usage of that is in tox's invocation for CI

Ideally, isort should produce some sort of output when it modifies things, the best case scenario I can see from this issue is someone (we?) contribute back some fixes to isort itself to improve the situation there.

Here's the original PR which added this btw: #305

@asottile
Copy link
Member

asottile commented Jan 9, 2018

(also (shameless plug) you may find reorder_python_imports to have more pre-commit friendly output -- though it has different configuration (and way fewer options (intentionally?)) than isort)

@bagerard
Copy link
Contributor Author

bagerard commented Jan 9, 2018

OK, I'll do a PR in isort and hope it will be integrated. I agree that it should be in isort anyway. I actually started looking at reorder_python_import first, but then switched to isort due to asottile/reorder-python-imports#8
Thanks for the quick feedback!

@bagerard bagerard closed this as completed Jan 9, 2018
@asottile
Copy link
Member

asottile commented Jan 9, 2018

Cool, if you want any help working on isort I've poked their codebase a few times and might be able to help with a PR :)

@bagerard
Copy link
Contributor Author

bagerard commented Jan 9, 2018

That's very nice of you but I was interested in implementing that (minor) feature. I just opened a PR in isort, let's see if it gets accepted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants