-
Notifications
You must be signed in to change notification settings - Fork 17
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: add truncate function #41
feat: add truncate function #41
Conversation
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.
I loved the PR, implementation and API on point! I just left some comments about overall code styling and organization to preserve a good standard in the repo
I appreciate the time spent in here!
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.
Amazing work @p9f ! 🎉
I'll just play around with it to think about how some of the decisions here feels,
especially this:
type test3 = Expect<Equal<Subject.Truncate<'Hello, world', 2>, '...'>>
I don't really have a clear opinion about it though 😅 . I just want to avoid changing the API like I'll have to do for other decisions I took too early.
As soon as I have the confidence this is the right API I'll make sure to merge it in time for v1.2
@gustavoguichard please take all your time to play with it first, there is no rush + I agree having to change the behaviour later is always annoying. for this: type test3 = Expect<Equal<Subject.Truncate<'Hello, world', 2>, '...'>> I don't have a strong opinion on this - I just followed lodash implementation which behave like this (agreed we don't always want to follow lodash, but i think in this case it might makes sense?). |
Yeah, it kind of makes sense. Also, following lodash is the easier path as it is what people are used to. |
src/utils.ts
Outdated
): Truncate<T, S, P> { | ||
if (length <= 0) return omission as Truncate<T, S, P> | ||
if (sentence.length <= length) return sentence as Truncate<T, S, P> | ||
if (sentence.length <= omission.length) return omission as Truncate<T, S, P> |
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.
Upon closer inspection this line seems unnecessary and also deviates from the type-level implementation.
By removing it we have the exact same implementation at both type and runtime levels =)
if (sentence.length <= omission.length) return omission as Truncate<T, S, P> |
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.
good point - done in 14bebeb
src/utils.ts
Outdated
length: S, | ||
omission = '...' as P, | ||
): Truncate<T, S, P> { | ||
if (length <= 0) return omission as Truncate<T, S, P> |
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.
Another lil adjustment to have the exact same implementation
if (length <= 0) return omission as Truncate<T, S, P> | |
if (length < 0) return omission as Truncate<T, S, P> |
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.
done in 14bebeb
Aside from @gustavoguichard 's oustanding comments, looks great 👍 |
@p9f just waiting for those 2 line changes and we are good to merge it |
roadmap mention we might want to implement a
truncate
function, similar to lodash one, here is a proposition.I think it behave as the lodash one, except for the
separator
option.I did not add the
separator
option, because: