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

750: Teach ad-hoc hooks line parsing #757

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

guillaume-d
Copy link

Include some (partly regression) tests and a small doc.

Copy link
Owner

@sds sds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for opening this pull request, @guillaume-d. Very excited to move this forward.

Left a few comments to clarify how we could potentially make this experience a seamless one without having to introduce the concept of an "ad hoc" hook to the configuration format. Curious to get your perspective.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
lib/overcommit/hook_loader/base.rb Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Owner

@sds sds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sincere apologies for the very slow response, @guillaume-d. Let me know your thoughts!

README.md Outdated Show resolved Hide resolved
lib/overcommit/hook_loader/base.rb Outdated Show resolved Hide resolved
@guillaume-d
Copy link
Author

Sincere apologies for the very slow response, @guillaume-d. Let me know your thoughts!

No problem, was started to worry something bad may have happened.
In the meantime, I have been implementing some kind of flattening, I will just finish it (hopefully today) and push that for now as a stepping stone, and once done will have another look at implementing your suggestions.

@guillaume-d guillaume-d force-pushed the guillaume-d/bug/750-teach-ad-hoc-hooks-line-parsing branch from 1438fa3 to 34bf2c8 Compare October 11, 2021 18:06
@guillaume-d guillaume-d force-pushed the guillaume-d/bug/750-teach-ad-hoc-hooks-line-parsing branch 3 times, most recently from f5f0d60 to 72ed54f Compare October 11, 2021 18:29
@guillaume-d guillaume-d force-pushed the guillaume-d/bug/750-teach-ad-hoc-hooks-line-parsing branch from 72ed54f to 3dc1d14 Compare October 11, 2021 18:31
@guillaume-d
Copy link
Author

Sincere apologies for the very slow response, @guillaume-d. Let me know your thoughts!

No problem, was started to worry something bad may have happened. In the meantime, I have been implementing some kind of flattening, I will just finish it (hopefully today) and push that for now as a stepping stone, and once done will have another look at implementing your suggestions.

Hi. I think all comments have finally been adressed now.

I am afraid I will not have much more time to do another review round (except maybe if feedback is very minor and comes fast, before I get sidetracked by other things).

If nothing comes I will just use my fork of overcommit going forward, and use #758 and future uses of this feature from there.

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

Successfully merging this pull request may close these issues.

2 participants