sds / overcommit

A fully configurable and extendable Git hook manager
MIT License
3.92k stars 280 forks source link

Standard and SemiStandard hooks misleadingly report success #231

Closed blech75 closed 9 years ago

blech75 commented 9 years ago

After enabling either Standard or SemiStandard precommit hooks, they misleadingly report success even though there are errors/warnings in the files being checked.

This is because standard v3.6.0 changed the output changed from stderr to stdout. (Both hooks currently capture output from stderr.) See feross/standard#112 (bug) and feross/standard#113 (PR) for background and discussion on the change. That change made it into semistandard v4.1.0 (via standard-engine v1.3.0) within a day's time which, coincidentally, was right around the release of overcommit 0.24.

The issue here is that stderr is now empty, and therefore is considered successful. Just need to change do output = result.stdout.chomp instead, for each hook.

Also, the regexp passed to extract_messages needs to change to account for spaces at the beginning of the line.

I'll submit the PR shortly.

blech75 commented 9 years ago

Also... How should the difference in output handling in the hook be accounted for? Meaning, should we support older versions of the tools? And if so, how? (My gut on this one says no.)

Hooks don't seem to have any notion of dependencies aside from required_executable and install_command. (Not that I think they necessarily should.) So it would be awkward to check for the installed version of the tool (standard or semistandard) and behave differently. I suppose any necessary difference in hook behaviors would be better implemented as a separate (custom) hook.

This issue happens to be simple (maybe it's an edge case), but I could imagine a larger change causing more problems.

Anyway, some food for thought.

jawshooah commented 9 years ago

I don't think optional per-hook version requirements would necessarily be a bad thing. SublimeLinter already performs similar checks, allowing individual linter plugins to optionally specify version_requirement, version_args (flags to pass to the executable) and version_regex (to parse the version number from the command output).

Probably not worth worrying about unless we run into more problems like this, but I don't believe it would require a huge effort to implement.

sds commented 9 years ago

Fixed by @blech75 in #232. Thanks for the report and accompanying fix!

Regarding adding support for version detection: I think it would be good to avoid this as long as possible. Adding a version check adds a delay to the hook run as we're invoking the executable twice (once the extract the version and then another to actually execute the command).