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

Improve DelimiterCase #930

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

filipw01
Copy link

@filipw01 filipw01 commented Aug 7, 2024

I made some changes that pass more test cases than the current solution. I was heavily inspired by CamelCase implementation

Opinions seem to be mixed from one library to another how hello123 case should be handled
Screenshot 2024-08-08 at 00 20 07
Even more interestingly, those libraries that default to hello_123 don't have an option to customize it, but those that default to hello123 have optional parameters to change that

I made the type customizable via a parameter to fit both cases

Related issues:
#223
#336

What I'd still like to add:

  • add more test cases for other cases (kebab-case etc.)

@filipw01
Copy link
Author

@voxpelli Could you take a look? At the moment I just want to know if this is something I should dig deeper into

@voxpelli
Copy link
Collaborator

@filipw01 Its not immediately obvious to me what the improvements are, and my experience with these helpers has been that they easily turn into a game of whackamole – any improvement reveals a couple of regressions.

I do see that you ultimate refer to my wish to unify the two: #224 (comment)

To support this use case I think we essentially would have to merge the functionality of the two. I can't remember why I didn't do that initially.

But that was ultimately solved through a new option instead: #501

I guess my main feedback would be to split this PR into more commits and try to make it as clear what and why the changes made are made, so that one can as easily as possible follow the reasoning in a review

@filipw01 filipw01 marked this pull request as draft August 27, 2024 13:06
@filipw01
Copy link
Author

filipw01 commented Nov 8, 2024

For the no split on number I decided to align with change-case library which seems to be the best at handling different edge cases

? [...SkipEmptyWord<CurrentWord>, ...SplitWords<RemainingCharacters, FirstCharacter, FirstCharacter>]
// Case change: numeric to non-numeric, push word
// Split on number: push word
? Options['splitOnNumber'] extends true
Copy link
Author

Choose a reason for hiding this comment

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

When there's a numeric/non-numeric case change I added and used a new option splitOnNumber which depending on how it is set will either

  1. When set to true - Like before split words on number
  2. When set to false - concat to current word

```

@category Change case
@category Template literal
*/
export type ScreamingSnakeCase<Value> = Value extends string
? IsScreamingSnakeCase<Value> extends true
Copy link
Author

Choose a reason for hiding this comment

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

I can bring this back, but I believe it shouldn't be necessary, especially that we don't use it for other things like snake case or kebab case

Comment on lines +52 to +77
const kebabFromMixed2: KebabCase<'parseHTML'> = 'parse-html';
expectType<'parse-html'>(kebabFromMixed2);

const kebabFromMixed3: KebabCase<'parseHTMLItem'> = 'parse-html-item';
expectType<'parse-html-item'>(kebabFromMixed3);

const kebabFromNumberInTheMiddleSplitOnNumber: KebabCase<'foo2bar'> = 'foo-2-bar';
expectType<'foo-2-bar'>(kebabFromNumberInTheMiddleSplitOnNumber);

const kebabFromNumberInTheMiddleSplitOnNumberEdgeCase: KebabCase<'foO2Bar'> = 'fo-o-2-bar';
expectType<'fo-o-2-bar'>(kebabFromNumberInTheMiddleSplitOnNumberEdgeCase);

const kebabFromNumberInTheMiddleSplitOnNumberEdgeCase2: KebabCase<'foO2bar'> = 'fo-o-2-bar';
expectType<'fo-o-2-bar'>(kebabFromNumberInTheMiddleSplitOnNumberEdgeCase2);

const kebabFromNumberInTheMiddleNoSplitOnNumber: KebabCase<'foo2bar', {splitOnNumber: false}> = 'foo2bar';
expectType<'foo2bar'>(kebabFromNumberInTheMiddleNoSplitOnNumber);

const kebabFromNumberInTheMiddleNoSplitOnNumberEdgeCase: KebabCase<'foo2Bar', {splitOnNumber: false}> = 'foo2-bar';
expectType<'foo2-bar'>(kebabFromNumberInTheMiddleNoSplitOnNumberEdgeCase);

const kebabFromNumberInTheMiddleNoSplitOnNumberEdgeCase2: KebabCase<'foO2bar', {splitOnNumber: false}> = 'fo-o2bar';
expectType<'fo-o2bar'>(kebabFromNumberInTheMiddleNoSplitOnNumberEdgeCase2);

const kebabFromNumberInTheMiddleNoSplitOnNumberEdgeCase3: KebabCase<'FOO22Bar', {splitOnNumber: false}> = 'foo22-bar';
expectType<'foo22-bar'>(kebabFromNumberInTheMiddleNoSplitOnNumberEdgeCase3);
Copy link
Author

Choose a reason for hiding this comment

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

I added the same tests to all case types as well as some edge cases visible here. If you have any doubts about the implementation I can add more tests

@@ -12,6 +13,7 @@ import type {KebabCase} from 'type-fest';
// Simple

const someVariable: KebabCase<'fooBar'> = 'foo-bar';
const someVariableNoSplitOnNumber: KebabCase<'p2pNetwork', {splitOnNumber: false}> = 'p2p-network';
Copy link
Author

Choose a reason for hiding this comment

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

I added this new option on case changing types

: Parts extends [string]
? string
: '';
type DelimiterCaseFromArray<
Copy link
Author

Choose a reason for hiding this comment

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

This gets the words split and inserts the delimited at the beginning and after every word

Like ['here', 'We', 'Go'] -> '#here#We#Go'

Delimiter
>
*/
export type DelimiterCase<Value, Delimiter extends string, Options extends SplitWordsOptions = {splitOnNumber: true}> = Value extends string
Copy link
Author

Choose a reason for hiding this comment

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

Here we

  1. 'hereWeGo' -> SplitWords -> ['here', 'We', 'Go']
  2. ['here', 'We', 'Go'] -> DelimiterCaseFromArray -> '#here#We#Go'
  3. '#here#We#Go' -> RemoveFirstLetter -> 'here#We#Go'
  4. 'here#We#Go' -> Lowercase -> 'here#we#go'

@filipw01
Copy link
Author

filipw01 commented Nov 8, 2024

@voxpelli At this point where all the commits are made I'd prefer not to do any git wizardry splitting into smaller commits. I left some comments for easier review. I hope this will be enough. If something is unclear let me know.

I tried to add more edge cases to avoid the whack a mole game

@filipw01 filipw01 marked this pull request as ready for review November 8, 2024 22:49
@voxpelli voxpelli mentioned this pull request Nov 10, 2024
@voxpelli
Copy link
Collaborator

#975 is also touching some of these files. I will not have time to review either, just noting it

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.

2 participants