Repository navigation
Teach ad-hoc hooks line-parsing using regexes in YAML #750
Description
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! 😉
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?
Activity
sds commentedon May 31, 2021
Supportive of reducing the friction to integration a separate executable/script with Overcommits line handling functionality. Pull request with tests welcome!
guillaume-d commentedon Jun 1, 2021
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, andplugin_hook_loader.rbwould 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.rbtests 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... 😉
So would only indirect testing as outlined above be fine?
sds commentedon Jun 5, 2021
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 commentedon Jun 10, 2021
Not sure I will follow up on this, I am mostly interested in tools that overcommit still does not support! 😛
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. 😉
guillaume-d commentedon Jun 11, 2021
See #757!
Complete configurable command diagnostics and cover issue sds#750 req…
750: Teach ad-hoc hooks line parsing (sds#757)