Skip to content

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

Merged
escapedcat merged 2 commits intoconventional-changelog:masterfrom
hasghari:fix-sentence-case
Oct 27, 2021
Merged

refactor(cz-commitlint,prompt): remove duplication in case conversion#2794
escapedcat merged 2 commits intoconventional-changelog:masterfrom
hasghari:fix-sentence-case

Conversation

@hasghari
Copy link
Copy Markdown
Contributor

@hasghari hasghari commented Oct 8, 2021 •
edited
Loading

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
hasghari commented Oct 8, 2021
View reviewed changes
Comment thread @commitlint/ensure/src/case.ts Outdated
case 'sentence-case':
case 'sentencecase':
return input.charAt(0).toUpperCase() + input.slice(1);
return upperFirst(input);
Copy link
Copy Markdown
Contributor Author

@hasghari hasghari Oct 8, 2021

Choose a reason for hiding this comment

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

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

Comment thread @commitlint/cz-commitlint/src/utils/case-fn.ts Outdated
case 'sentencecase':
return (input: string) =>
`${input.charAt(0).toUpperCase()}${input.substring(1).toLowerCase()}`;
return (input: string) => upperFirst(input);
Copy link
Copy Markdown
Contributor Author

@hasghari hasghari Oct 8, 2021

Choose a reason for hiding this comment

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

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

@escapedcat
Copy link
Copy Markdown
Member

escapedcat commented Oct 8, 2021 •
edited
Loading

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
Copy link
Copy Markdown
Contributor Author

hasghari commented Oct 8, 2021

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
Copy link
Copy Markdown
Member

escapedcat commented Oct 9, 2021

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
Copy link
Copy Markdown
Contributor Author

hasghari commented Oct 9, 2021 •
edited
Loading

@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.

@escapedcat escapedcat requested a review from AdeAttwood October 10, 2021 03:06
@curly210102
Copy link
Copy Markdown
Contributor

curly210102 commented Oct 11, 2021 •
edited
Loading

@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
Copy link
Copy Markdown
Contributor Author

hasghari commented Oct 11, 2021

@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.

@curly210102 curly210102 mentioned this pull request Oct 12, 2021
Merged
7 tasks
@escapedcat
Copy link
Copy Markdown
Member

escapedcat commented Oct 26, 2021

@hasghari would you mind rebasing this? Thanks!

@hasghari
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.
@hasghari
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 hasghari changed the title fix: align sentence-case implementation refactor(cz-commitlint,prompt): remove duplication in case conversion Oct 26, 2021
@hasghari
Copy link
Copy Markdown
Contributor Author

hasghari commented Oct 26, 2021

@hasghari would you mind rebasing this? Thanks!

Done.

@escapedcat
Copy link
Copy Markdown
Member

escapedcat commented Oct 27, 2021

Thanks @hasghari ! Moving this in.

@escapedcat escapedcat merged commit 7841a5d into conventional-changelog:master Oct 27, 2021
@hasghari hasghari deleted the fix-sentence-case branch October 27, 2021 01:19
@escapedcat
Copy link
Copy Markdown
Member

escapedcat commented Nov 1, 2021

It's released with 14.0.0/14.1.0

@escapedcat escapedcat mentioned this pull request Feb 5, 2023
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AdeAttwood AdeAttwood Awaiting requested review from AdeAttwood

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@hasghari @escapedcat @curly210102