refactor(cz-commitlint,prompt): remove duplication in case conversion #2794

hasghari · 2021-10-08T01:44:08Z

Description

The implementation of sentence-case for cz-commitlint differs from the one in @commitlint/ensure/src/case.ts. Ideally the @commitlint/cz-commitlint package should depend on the @commitlint/ensure package and import the toCase function to remove the duplicate implementation but admittedly I'm not sure how to accomplish that in a monorepo like this. Does that require submitting a separate pull request for @commitlint/ensure to export the toCase function?

Motivation and Context

Let's say my desired commit message is "ci: Add GitHub Action for commitlint". If I author that manually, it passes commitlint. However with commitizen, it converts everything except the first character to lowercase, ending up with "ci: Add github action for commitlint"

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

hasghari · 2021-10-08T01:45:39Z

@commitlint/ensure/src/case.ts

@@ -43,7 +43,7 @@ function toCase(input: string, target: TargetCaseType): string {
 			return input.toUpperCase();
 		case 'sentence-case':
 		case 'sentencecase':
-			return input.charAt(0).toUpperCase() + input.slice(1);
+			return upperFirst(input);


This is functionally equivalent to the current implementation as far as I can tell.

hasghari · 2021-10-08T01:46:58Z

@commitlint/cz-commitlint/src/utils/case-fn.ts

@@ -42,8 +42,7 @@ export default function getCaseFn(rule?: Rule): CaseFn {
 			return (input: string) => input.toUpperCase();
 		case 'sentence-case':
 		case 'sentencecase':
-			return (input: string) =>
-				`${input.charAt(0).toUpperCase()}${input.substring(1).toLowerCase()}`;
+			return (input: string) => upperFirst(input);


The implementation here deviates from the one in @commitlint/ensure/src/case.ts

escapedcat · 2021-10-08T01:50:39Z

Hey @hasghari , thanks for this PR!
Would be nice if you could fill out "Description" and "Motivation and Context".

Edit: Ah. you're on it right now ;) Thanks!

hasghari · 2021-10-08T12:12:16Z

Hey @hasghari , thanks for this PR! Would be nice if you could fill out "Description" and "Motivation and Context".

Edit: Ah. you're on it right now ;) Thanks!

Updated with description and context.

escapedcat · 2021-10-09T08:21:12Z

Hey @hasghari , thanks for pointing out this issue and explaining it :)

Ideally the @commitlint/cz-commitlint package should depend on the @commitlint/ensure package and import the toCase function to remove the duplicate implementation but admittedly I'm not sure how to accomplish that in a monorepo like this. Does that require submitting a separate pull request for @commitlint/ensure to export the toCase function?

No need to worry about the monorepo thing. You could just export the function and use it in the cz-commitlint package.
You wanna give that a try? If you get stuck I could try to help you out.
That might be a clearer solution instead of copying the functionality.
@curly210102 what do you think?

hasghari · 2021-10-09T20:07:21Z

@escapedcat I took a stab at removing the duplication. I also left my earlier commit in place but I'd be happy to squash it.

curly210102 · 2021-10-11T02:28:14Z

@escapedcat @hasghari It's good to extract to-case, reuse in commitlint, @commitlint/cz-commitlint and @commitlint/prompt. I propose to fix this issue in cz-commitlint with a quick patch, meanwhile waiting for this PR review.

hasghari · 2021-10-11T12:36:25Z

@escapedcat @hasghari It's good to extract to-case, reuse in commitlint, @commitlint/cz-commitlint and @commitlint/prompt. I propose to fix this issue in cz-commitlint with a quick patch, meanwhile waiting for this PR review.

Thanks for pointing out the @commitlint/prompt package. I've added another commit to remove the duplication there as well.

escapedcat · 2021-10-26T05:01:26Z

@hasghari would you mind rebasing this? Thanks!

The @commitlint/cz-commitlint was previously duplicating the case conversion logic also found in the @commitlint/ensure package.

The @commitlint/prompt package was previously duplicating the case conversion logic also found in the @commitlint/ensure package.

hasghari · 2021-10-26T12:42:59Z

@hasghari would you mind rebasing this? Thanks!

Done.

escapedcat · 2021-10-27T01:10:28Z

Thanks @hasghari ! Moving this in.

escapedcat · 2021-11-01T06:23:52Z

It's released with 14.0.0/14.1.0

hasghari commented Oct 8, 2021

View reviewed changes

hasghari force-pushed the fix-sentence-case branch from f81f1c5 to cfce62c Compare October 8, 2021 12:21

hasghari force-pushed the fix-sentence-case branch from 7337683 to a416168 Compare October 9, 2021 20:06

escapedcat requested a review from AdeAttwood October 10, 2021 03:06

hasghari force-pushed the fix-sentence-case branch from a416168 to 5350795 Compare October 11, 2021 12:28

curly210102 mentioned this pull request Oct 12, 2021

support multiple scopes and multiple cases & fix sentence-case is not consistent with commitlint/cli #2806

Merged

7 tasks

hasghari added 2 commits October 26, 2021 06:36

refactor(cz-commitlint): remove duplication in case conversion

65a4991

The @commitlint/cz-commitlint was previously duplicating the case conversion logic also found in the @commitlint/ensure package.

refactor(prompt): remove duplication in case conversion

5199ccf

The @commitlint/prompt package was previously duplicating the case conversion logic also found in the @commitlint/ensure package.

hasghari force-pushed the fix-sentence-case branch from 5350795 to 5199ccf Compare October 26, 2021 12:39

hasghari changed the title ~~fix: align sentence-case implementation~~ refactor(cz-commitlint,prompt): remove duplication in case conversion Oct 26, 2021

escapedcat merged commit 7841a5d into conventional-changelog:master Oct 27, 2021

hasghari deleted the fix-sentence-case branch October 27, 2021 01:19

escapedcat mentioned this pull request Feb 5, 2023

Sentence-case is not work properly #3517

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(cz-commitlint,prompt): remove duplication in case conversion #2794

refactor(cz-commitlint,prompt): remove duplication in case conversion #2794

hasghari commented Oct 8, 2021 •

edited

Loading

hasghari Oct 8, 2021

hasghari Oct 8, 2021

escapedcat commented Oct 8, 2021 •

edited

Loading

hasghari commented Oct 8, 2021

escapedcat commented Oct 9, 2021

hasghari commented Oct 9, 2021 •

edited

Loading

curly210102 commented Oct 11, 2021 •

edited

Loading

hasghari commented Oct 11, 2021

escapedcat commented Oct 26, 2021

hasghari commented Oct 26, 2021

escapedcat commented Oct 27, 2021

escapedcat commented Nov 1, 2021

refactor(cz-commitlint,prompt): remove duplication in case conversion #2794

refactor(cz-commitlint,prompt): remove duplication in case conversion #2794

Conversation

hasghari commented Oct 8, 2021 • edited Loading

Description

Motivation and Context

Types of changes

Checklist:

hasghari Oct 8, 2021

Choose a reason for hiding this comment

hasghari Oct 8, 2021

Choose a reason for hiding this comment

escapedcat commented Oct 8, 2021 • edited Loading

hasghari commented Oct 8, 2021

escapedcat commented Oct 9, 2021

hasghari commented Oct 9, 2021 • edited Loading

curly210102 commented Oct 11, 2021 • edited Loading

hasghari commented Oct 11, 2021

escapedcat commented Oct 26, 2021

hasghari commented Oct 26, 2021

escapedcat commented Oct 27, 2021

escapedcat commented Nov 1, 2021

hasghari commented Oct 8, 2021 •

edited

Loading

escapedcat commented Oct 8, 2021 •

edited

Loading

hasghari commented Oct 9, 2021 •

edited

Loading

curly210102 commented Oct 11, 2021 •

edited

Loading