-
Notifications
You must be signed in to change notification settings - Fork 15
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(utils): add string helper #916
base: main
Are you sure you want to change the base?
Conversation
Code PushUp🤨 Code PushUp report has both improvements and regressions – compared target commit bef6ec1 with source commit 6a74c2d. 🕵️ See full comparison in Code PushUp portal 🔍 🏷️ Categories👍 1 group improved, 👎 1 group regressed, 👍 3 audits improved, 👎 4 audits regressed, 12 audits changed without impacting score🗃️ Groups
15 other groups are unchanged. 🛡️ Audits
569 other audits are unchanged. |
* @param string - The slug to format. | ||
* @returns The formatted title. | ||
*/ | ||
export function kebabCaseToSentence(string = '') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since export function kebabCaseToCamelCase(string: string) {
the param it's not optional, I would not do this optional either
export function kebabCaseToSentence(string = '') { | |
export function kebabCaseToSentence(string: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. There would be no sense in calling kebabCaseToSentence()
without arguments. Same goes for camelCaseToSentence()
.
* @param string - The slug to format. | ||
* @returns The formatted title. | ||
*/ | ||
export function camelCaseToSentence(string = '') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is missing unit tests.
describe('kebabCaseToSentence', () => { | ||
it('should convert simple slug to title case', () => { | ||
expect(kebabCaseToSentence('hello-world')).toBe('Hello World'); | ||
}); | ||
|
||
it('should handle multiple hyphens', () => { | ||
expect(kebabCaseToSentence('this-is-a-title')).toBe('This Is A Title'); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you've implemented is called title case, not sentence case. See Title Case vs. Sentence Case: What’s the Difference?:
Title case and sentence case are two different styles of capitalization used in writing titles, headings, and headlines. With title case, the first letter of every major word is capitalized, while articles, conjunctions, or prepositions are lowercase—unless they are the first word in the title. In sentence case, only the first word is capitalized, along with any proper nouns in the title or heading.
- Title case example: The Mystery of the Missing Red Ruby
- Sentence case example: The mystery of the missing red ruby
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe string.ts
a bit too broad? 🤔 How about casings.ts
or case-conversions.ts
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a capitalize
function in transform.ts
. Since it's also about converting string case, it could be moved in here.
export type CamelCaseToKebabCase<T extends string> = | ||
T extends `${infer First}${infer Rest}` | ||
? Rest extends Uncapitalize<Rest> | ||
? `${Lowercase<First>}${CamelCaseToKebabCase<Rest>}` | ||
: `${Lowercase<First>}-${CamelCaseToKebabCase<Rest>}` | ||
: T; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The inferred type doesn't match the returned string from camelCaseToKebabCase
:
It's important (for developer sanity) that the type is consistent with reality, even if that means making the function less "robust". For example, the 'hello_world test'
input string isn't in camelCase, so converting it need not be the function's responsibility. After all, the function is called camelCaseToKebabCase
, it doesn't claim to convert anything to kebab-case. Simplifying the runtime conversion will also simplify the type conversion - reimplementing the current conversion in types would very challenging otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, Vitest supports Testing Types. We should consider that for these complex types. I actually used it recently here and it works really well, all I had to do was add --typecheck
to vitest
command. We could leave that for another PR, though.
* @param string - The kebab-case string to convert. | ||
* @returns The camelCase string. | ||
*/ | ||
export function kebabCaseToCamelCase(string: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the inverse function uses a CamelCaseToKebabCase<T>
, why not create a KebabCaseToCamelCase<T>
type also? It's actually a lot easier to do the conversion in this direction:
export type KebabCaseToCamelCase<T extends string> =
T extends `${infer First}-${infer Rest}`
? `${First}${Capitalize<KebabCaseToCamelCase<Rest>>}`
: T;
This PR includes helper for the TypeScript plugin #902