-
-
Notifications
You must be signed in to change notification settings - Fork 123
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: Add ability to use multiple scopes #85
Conversation
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.
Hey @amromusharaf,
thank you for this excellent pull request! 💯
I only left some minor comments, maybe give them your consideration.
Do you know if other tools like semantic-release
support multiple scopes as well? The combination with that tool was at least my initial motivation to build this action. If yes, maybe it's worth mentioning this feature in the README with the validation examples.
Otherwise I'd silently support it the way you've implemented it right now. Otherwise a user might get the impression that this syntax is generally supported.
Thanks!
Hey @amannn -- thanks for your quick response! The changes you've requested sound great; I've pushed them, and I appreciate your thoughtful review :) As to the question re: https://www.conventionalcommits.org/en/v1.0.0/#specification I think, as you say, it may be best to silently support it for the time being? Thanks again for your time, by the way -- it's been a pleasure contributing to your codebase :) |
True, thank you for the links. It seems like commit lint also supports the same syntax already: conventional-changelog/commitlint#901.
Yep, sounds fair!
Great to hear! Thank you for your contribution 👏 |
🎉 This PR is included in version 3.3.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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.
any issue in convention of comma separator symbol ,
with—or without—a trailing space?
|
||
it('throws when an unknown scope is detected within multiple scopes', async () => { | ||
await expect( | ||
validatePrTitle('fix(core,e2e,foo,bar): Bar', {scopes: ['foo', 'core']}) |
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.
any issue in convention of comma separator symbol ,
with—or without—a trailing space?
I remember one CLI tool parsed differently to the other (replacing spaces with hyphen), not sure if the push made it implicitly to the "official spec" https://www.conventionalcommits.org/en/v1.0.0-beta.3/
has seen adoption from enough tools
conventional-changelog/conventional-changelog#232
conventional-changelog/commitlint#901
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.
Not sure about the official convention, but I think we should support spaces in this action:
? result.scope.split(',').map((scope) => scope.trim()) |
This PR adds the oft-requested ability to use multiple scopes, and includes tests. For example:
feat(foo,bar): Add cool new feature