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

build: use @commitlint/cz-commitlint instead of cz-conventional-changelog #7264

Merged
merged 1 commit into from
May 24, 2021
Merged

build: use @commitlint/cz-commitlint instead of cz-conventional-changelog #7264

merged 1 commit into from
May 24, 2021

Conversation

johnpc
Copy link
Contributor

@johnpc johnpc commented May 5, 2021

Description of changes

This pull request is a proposal that can be either taken or left. I got confused when I used yarn commit but no indication of valid scopes was provided during that potion of interaction. This resulted in me using an invalid scope, failing the commit, and re-running yarn commit with a valid scope from the failure error message.

A lerna scopes enum (from @commitlint/config-lerna-scopes) is currently enforced by commitlint. However, commitizen is not configured to be aware of that list of valid scopes. This makes it possible to call yarn commit and result in the generated commit failing the lint rule:

cz-conventional-changelog mov

This pull request removes the dependency on cz-conventional-changelog and installs @commitlint/cz-commitlint as an alternative. @commitlint/cz-commitlint shares it's config with commitlint.config.js, which gives it access to @commitlint/config-lerna-scopes as selectable scopes without changing the interactive commit experience.

cz-commitlint mov

Further motivation is explained in conventional-changelog/commitlint#2547

The downside of this change is that the scope of the change must be selected via arrow key-ing down to the scope you want. It might be faster for regular contributors to manually type their scope than scroll potentially very far down the list to find the desired scope.

If the current mechanism is preferred, feel free to close this PR. If the change is deemed valuable, let me know if anything else is needed to get it shipped.

Description of how you validated changes

I committed this very change on my fork in order to create this pull request

Checklist

  • PR description included

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@johnpc johnpc requested a review from a team as a code owner May 5, 2021 19:26
@johnpc johnpc changed the title build: use @commitlint/cz-commitlint instead of cz-conventional-chang elog build: use @commitlint/cz-commitlint instead of cz-conventional-changelog May 5, 2021
@cjihrig
Copy link
Contributor

cjihrig commented May 11, 2021

Thank you for the contribution. I've personally been bitten by this before. That said, I think I'd personally rather not merge this due to the interactive wizard that it introduces. Furthermore, I think we can avoid the issue completely by not adding a scope in the commit message. Lerna is smart enough to figure out which module(s) changed when generating the changelog.

Is there anyone from @aws-amplify/amplify-cli with strong feelings one way or the other here?

@johnpc
Copy link
Contributor Author

johnpc commented May 11, 2021 via email

@cjihrig
Copy link
Contributor

cjihrig commented May 17, 2021

Closing, as we discussed this during the meeting. We would like to make the committing process simpler and add validation around squashing if possible.

@cjihrig cjihrig closed this May 17, 2021
@johnpc johnpc reopened this May 18, 2021
@johnpc
Copy link
Contributor Author

johnpc commented May 18, 2021

reopening - I think there was a misunderstanding that this wizard would remove the ability to author messages using git commit. If you use git commit, this PR does not impact your workflow. This PR simply fixes issues with the currently existing yarn commit wizard.

@cjihrig
Copy link
Contributor

cjihrig commented May 18, 2021

@johnpc and I discussed this on Slack. I'm OK with the changes to yarn commit here.

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM with the merge conflicts resolved.

…elog

This commit updates to @commitlint/cz-commitlint, which adds the `scope` enum from commitlint
without changing the interactive commit experience. Further motivation is explained in
conventional-changelog/commitlint#2547
@yuth yuth merged commit 5b0fd05 into aws-amplify:master May 24, 2021
cjihrig pushed a commit to ctjlewis/amplify-cli that referenced this pull request Jul 12, 2021
…elog (aws-amplify#7264)

This commit updates to @commitlint/cz-commitlint, which adds the `scope` enum from commitlint
without changing the interactive commit experience. Further motivation is explained in
conventional-changelog/commitlint#2547

Co-authored-by: John Corser <xss@amazon.com>
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.

3 participants