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

feat: Add ability to use multiple scopes #85

Merged
merged 4 commits into from
Feb 10, 2021

Conversation

amromusharaf
Copy link
Contributor

@amromusharaf amromusharaf commented Feb 8, 2021

This PR adds the oft-requested ability to use multiple scopes, and includes tests. For example:

feat(foo,bar): Add cool new feature

Copy link
Owner

@amannn amannn left a 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!

src/validatePrTitle.js Outdated Show resolved Hide resolved
src/validatePrTitle.test.js Outdated Show resolved Hide resolved
src/validatePrTitle.js Outdated Show resolved Hide resolved
@amromusharaf
Copy link
Contributor Author

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: semantic-release, I'm unsure whether or not they support multiple scopes, though I'd be a little surprised if they do, as I'm not sure this is part of the official spec just yet. That being said, I noticed it's been requested in a few places, which is why I ultimately ended up opening this PR:

https://www.conventionalcommits.org/en/v1.0.0/#specification
conventional-commits/conventionalcommits.org#302
conventional-changelog/commitlint#75
conventional-changelog/conventional-changelog#232

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 :)

@amannn
Copy link
Owner

amannn commented Feb 10, 2021

I noticed it's been requested in a few places

True, thank you for the links. It seems like commit lint also supports the same syntax already: conventional-changelog/commitlint#901.

I think, as you say, it may be best to silently support it for the time being?

Yep, sounds fair!

it's been a pleasure contributing to your codebase :)

Great to hear! Thank you for your contribution 👏

@amannn amannn merged commit d6aabb6 into amannn:master Feb 10, 2021
@github-actions
Copy link

🎉 This PR is included in version 3.3.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Copy link

@webstoreportal webstoreportal left a 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']})
Copy link

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

Copy link
Owner

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())

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.

None yet

3 participants