-
Notifications
You must be signed in to change notification settings - Fork 29
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
Alter and/or document hook logic #103
Comments
@joshuatz this is definitely an annoying shortcoming of RDM's hooks; I've run into it a bunch of times. @russellkan and @joshuatz , what do you guys think of only creating issues for dash-separated numbers at the start of the branch. Thus: 1-asdf --> 1 Also, I think describing this in the README is a good idea. |
I think that makes sense. It catches pretty much any case of numerals that aren't intended to be part of the issue listing, and adding a description to the README would definitely be helpful for users. |
^ Agreed. Plus, it seems like enforcing a standard naming pattern for branches is a best practice anyways, and as a user I am usually happy to give up a little flexibility in exchange for more predicable behavior. |
Great, if either of you feel like implementing this, that would be great! There is an internal project in Harvest for RDM. I added both of you to it. |
I'm up for taking it on - sounds like fun! I'll probably get to this tomorrow or over the weekend. |
- Alter issue # extraction regex in the prepare-commit-msg hook. Only accept issue numbers at beginning of string, avoiding mixed non-issue number issues. - Update hooks test
I've started using the git commit hooks provided by RDM and ran into an unexpected outcome today. I'm not sure I would classify it as a bug, but I think it might not match assumptions made by users and should probably either be updated or perhaps even just documented with a warning somewhere.
Issue
Given a branch name of
245-fix-uploads-to-s3
, the commit message will be pre-populated with:I certainly didn't intend for
Issue #3
to get linked, nor do I think most users would.I suppose this probably doesn't happen that often, but could still see it happening more than once, with branch names like
__-migrate-to-python-v3
,__-implement-s3
,__-fix-base64-usage
, etc.Cause
Looks like the pattern for extracting the issue is pretty broad (
[0-9]+
).Proposed solution
Although users can easily edit the script themselves, it might be nice to either call this out somewhere in the docs, or refine the regex a little to provide a different default.
For example, although not very nice to look at (this was a quick pass; could probably be refined more), this avoids the above issue, as well as some other edge-cases:
Test cases here.
The text was updated successfully, but these errors were encountered: