Adds support for 'log_file' in hook config #530
Conversation
pre_commit/commands/run.py
Outdated
| @@ -121,7 +121,8 @@ def _run_single_hook(hook, repo, args, skips, cols): | |||
| for out in (stdout, stderr): | |||
| assert type(out) is bytes, type(out) | |||
| if out.strip(): | |||
| output.write_line(out.strip()) | |||
| output.write_line(out.strip(), | |||
| logfile_name=hook.get('log_file')) | |||
There was a problem hiding this comment.
(also applies to the string literal below)
pre_commit/output.py
Outdated
|
|
||
| with ExitStack() as stack: | ||
| try: | ||
| logfile = stack.enter_context(open(logfile_name, 'wb')) |
There was a problem hiding this comment.
this is going to clobber the contents of the file on each write (probably not what you want?)
setup.py
Outdated
| @@ -41,6 +41,7 @@ | |||
| install_requires=[ | |||
| 'aspy.yaml', | |||
| 'cached-property', | |||
| 'contextlib2', | |||
There was a problem hiding this comment.
I actually run a youtube channel and my next episode in "porting to python 3" is about proper use of backports. Haven't written up the slides yet or I'd link them but for backported modules you only want to conditionally install them on the versions that need them (otherwise we're installing contextlib2 in python3 where it's entirely unnecessary).
Here's the basic gist though: https://github.com/pre-commit/pre-commit/blob/v0.8.2/setup.py#L50-L52
pre_commit/output.py
Outdated
| try: | ||
| from contextlib import ExitStack | ||
| except ImportError: | ||
| from contextlib2 import ExitStack |
There was a problem hiding this comment.
when possible, prefer putting conditional imports after all the other imports (the import reorderer stops when it sees non-import code)
tests/commands/run_test.py
Outdated
| raise TypeError() | ||
| return mock.DEFAULT | ||
|
|
||
| with mock.patch('pre_commit.output.open', mock.mock_open()) as mocked_open: |
There was a problem hiding this comment.
I'd rather see a real integration test which the file is actually written -- especially since the code as written doesn't properly write the file -- There's fixtures in testing/fixtures for setting up a git repository (and fixtures in testing/conftest.py as well)
|
Thanks for the helpful feedback. Please take a look at the changes. |
pre_commit/output.py
Outdated
| stream.write(b'\n') | ||
| stream.flush() | ||
| def write_line(s=None, stream=stdout_byte_stream, logfile_name=None): | ||
| output_streams = [stream, ] |
setup.py
Outdated
| @@ -46,6 +46,9 @@ | |||
| 'six', | |||
| 'virtualenv', | |||
| ], | |||
| extras_require={ | |||
| ':python_version<"3.0"': ['contextlib2', ], | |||
pre_commit/output.py
Outdated
| logfile = stack.enter_context(open(logfile_name, 'ab')) | ||
| output_streams.append(logfile) | ||
| except (TypeError, IOError): | ||
| pass |
There was a problem hiding this comment.
did you try the noop ctx idea from the first review? what did you think?
There was a problem hiding this comment.
Oh wut, my original comment never posted! lol
Here's an idea that avoids needing contextlib2 (we use this pattern elsewhere in pre-commit too!)
if log_file:
ctx = open(...)
else:
ctx = noop_ctx()
with ctx:
...Specify a filename on a per hook basis and pre-commit will write the STDOUT and STDERR of that hook into the file. Useful for CI. Resolves #499.
|
Here's a way of doing it without contextlib2. |
|
Thanks! I'll touch this up and put it into the next release :) |
|
Thanks Anthony. And thanks for the awesome software! I received a github comment as an email but I don't see it in github itself:
My answer to that is, sure, go ahead 👍 . I'm not sure what you are referring to, though. Did I put the schema definition in the wrong place? |
|
Ah yeah I deleted the comment as I'll handle it better at a later date. Sorry for the email spam! |
Add --additional-domain to check-vcs-permalinks
Specify a filename on a per hook basis and
pre-commit will write the STDOUT and STDERR
of that hook into the file. Useful for CI.
Resolves #499.