Teach ad-hoc hooks line-parsing using regexes in YAML

[guillaume-d]

Many non-ad-hoc (pre-commit) hooks seem just to get the error code and parse stdout using extract_messages with the right MESSAGE_REGEX and MESSAGE_TYPE_CATEGORIZER.

Ad-hoc hooks could be taught how to do that from the overcommit YAML config file using:

• one regex for the message (a good default being the Emacs format)
• one optional regex to test whether the message is a warning or an error ("warning:" seems pretty common)
• maybe one optional regex to prefilter lines (empty lines, non-standard line, etc.), and flags to capture stdout and/or stderr, if we really need or want to handle more cases

Of course I guess in the end it all boils down to whether one prefers Ruby to regexes! :wink: Anyway this might still be a good alternative for non-Ruby programmers to easily contribute hooks, and a good way to write less code (although I am not sure yet how tests could be written).

Thoughts?

[sds]

Supportive of reducing the friction to integration a separate executable/script with Overcommits line handling functionality. Pull request with tests welcome!

[guillaume-d]

OK, thanks for confirming!

Another question: is there a good reason why built-in hooks could not "be ad-hoc" like plugin hooks? If there is no particular reason I'd like to change that too: it would make the new ad-hoc code easier to test: in the same PR one could change some built-in hooks to have an ad-hoc implementation, relying on the corresponding existing hook specs for (regression) testing. (In that case probably all 3 hook_loader/base.rb, built_in_hook_loader.rb, and plugin_hook_loader.rb would need changing.) Also doing the above would add some test coverage for the ad-hoc code which does not look covered at all at the moment.

As regards more targeted unit testing I cannot find an existing unit test which exercise the class(es) I want to modify: only cli_spec.rb tests HookRunner, which itself uses the hook loader class(es) I would like to test. Ideally one could reuse the existing hooks' spec code, somehow only override the overcommit configuration with ad-hoc hook definitions for a few hooks and synthetize new spec tests from that, but my actual rspec skills are much lower than my hand-waving ones here... :wink:

So would only indirect testing as outlined above be fine?

[sds]

I could see many built-in hooks converted to the ad hoc format, yes. I haven't considered the implications too deeply, however, but would be open to a proposal.

Perhaps we can reduce complexity while still bringing value by decoupling of rewriting existing hooks from the adding of support for built-in hooks to use an ad hoc format.

[guillaume-d]

I could see many built-in hooks converted to the ad hoc format, yes. I haven't considered the implications too deeply, however, but would be open to a proposal.

Not sure I will follow up on this, I am mostly interested in tools that overcommit still does not support! :stuck_out_tongue:

Perhaps we can reduce complexity while still bringing value by decoupling of rewriting existing hooks from the adding of support for built-in hooks to use an ad hoc format.

Yeah, I came to the same conclusion, and finally found a way to test at least some of the new code using rspec while implementing it.

However I also uncovered one glaring bug only after using it for real, while integrating a tool not yet supported by overcommit (to test my own changes in overcommit). I guess that integration may warrant a separate PR, I'll do at least separate commits for now.

I still need to cleanup the code (still a few Rubocop violations) and finish the doc. Next time I shall write less and let the code speak for itself. :wink:

[guillaume-d]

See https://github.com/sds/overcommit/pull/757!