-
Notifications
You must be signed in to change notification settings - Fork 41
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
feat: make the message RegEx configurable #162
base: master
Are you sure you want to change the base?
feat: make the message RegEx configurable #162
Conversation
Hi, thanks for the PR.
|
I updated the PR. The revert RegExes are bit out of scope. Those work intertwined with message and body. This should be a separate implementation. config.convention.msgRevertRegex = /^Revert +"'['"]/i; |
@@ -15,6 +15,10 @@ convention: | |||
- revert | |||
commitScopes: [] | |||
releaseTagGlobPattern: v[0-9]*.[0-9]*.[0-9]* | |||
msgRegex: /^(?<type>\w+)(?:\((?<scope>[^()]+)\))?(?<breaking>!)?:\s*(?<description>.+)/i |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not add those configs to the default config because it's an edge case. I would rather just add a section to the readme file for that edge case. Just because it makes default setup a little cumbersome.
@@ -15,6 +15,10 @@ convention: | |||
- revert | |||
commitScopes: [] | |||
releaseTagGlobPattern: v[0-9]*.[0-9]*.[0-9]* | |||
msgRegex: /^(?<type>\w+)(?:\((?<scope>[^()]+)\))?(?<breaking>!)?:\s*(?<description>.+)/i | |||
msgMergeRegexList: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed my mind, let's add other regex another time.
if (hasMandatoryCaptureGroups(conventionOverride.msgRegex, ['type', 'scope', 'breaking', 'description'])) { | ||
conventionConfig.msgRegex = conventionOverride.msgRegex; | ||
} else { | ||
console.warn(`[WARN] The message RegEx: ${conventionOverride.msgRegex} does not have the mandatory capture groups type, scope, breaking and description. The fallback is used`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to throw an error
We need to add a runtime check at the place where the regex will be used to ensure that if the regex matches that all mandatory capturing groups are not empty |
Ah okay. I understood the check wrong. I thought it would be enough in the parsing logic of the config. What should happen if the mandatory fields are not there? Right now it will throw a warn and will not put the commit on the changelog list. Isn't this enough? I'm on vacation until next week. I will add the changes then :) |
Closes #161