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

feat: add startsWith and endsWith #27

Merged
merged 8 commits into from
Oct 5, 2023
Merged

feat: add startsWith and endsWith #27

merged 8 commits into from
Oct 5, 2023

Conversation

jly36963
Copy link
Contributor

@jly36963 jly36963 commented Oct 4, 2023

No description provided.

: false // T (Text) is exhausted with S (Search) leftover
: StartsWith<Slice<T, P>, S, 0> // P is >0, slice
: StartsWith<T, S, 0> // P (Position) is negative, ignore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wishing for a more succinct way to express this type, but position creates a bunch of extra if/else blocks and corner cases

Copy link
Owner

Choose a reason for hiding this comment

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

Is positioning part of the native API? I didn't know about it.
In this case I think you can use Slice in to simplify the type implementation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is, but the way startsWith and slice handle negative indices is a little different

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2023-10-04 at 6 01 50 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to capture all these cases in the unit tests

Comment on lines 234 to 242
? P extends 0
? T extends `${infer TFirst}${infer TRest}`
? S extends `${infer SFirst}${infer SRest}`
? TFirst extends SFirst
? StartsWith<TRest, SRest, P> // Compare next character
: false // T differs from S
: true // S (Search) is exhausted (and didn't fail)
: false // T (Text) is exhausted with S (Search) leftover
: StartsWith<Slice<T, P>, S, 0> // P is >0, slice
Copy link
Owner

@gustavoguichard gustavoguichard Oct 4, 2023

Choose a reason for hiding this comment

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

Suggested change
? P extends 0
? T extends `${infer TFirst}${infer TRest}`
? S extends `${infer SFirst}${infer SRest}`
? TFirst extends SFirst
? StartsWith<TRest, SRest, P> // Compare next character
: false // T differs from S
: true // S (Search) is exhausted (and didn't fail)
: false // T (Text) is exhausted with S (Search) leftover
: StartsWith<Slice<T, P>, S, 0> // P is >0, slice
? Slice<T, P> extends `${S}${string}` ? true : false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oooooh I forgot that TS could do

`${S}${string}`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your suggestion won't work as-is because of the way startsWith handles negative position, but with a little bit of adaptation I can replace the existing type with something a bit shorter

@gustavoguichard
Copy link
Owner

Go on and implement endsWith along with it 😉

@jly36963
Copy link
Contributor Author

jly36963 commented Oct 5, 2023

Go on and implement endsWith along with it 😉

endsWith might need to wait until we figure out the negative index issue with slice. endsWith has a position param (endPosition) that might make things complicated with slice

I'll start a second PR with it if I can make it work. (PR on this branch, or on main if you accept/merge this one.)

@jly36963 jly36963 marked this pull request as ready for review October 5, 2023 00:21
@gustavoguichard
Copy link
Owner

I believe you can use Slice<S, 0, P> to implement endsWith, can't you?

You probably wouldn't need the negative as it is done here

@gustavoguichard
Copy link
Owner

@jly36963 I just merged the fix for slice and it now accepts negative indexes

README.md Outdated
@@ -100,6 +100,7 @@ npm install string-ts
- [split](#split)
- [toLowerCase](#tolowercase)
- [toUpperCase](#touppercase)
- [startsWith](#startsWith)
Copy link
Owner

Choose a reason for hiding this comment

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

Could you keep alphabetical order here? I think it is the only place where it was overlooked.

@jly36963 jly36963 marked this pull request as draft October 5, 2023 03:34
@jly36963 jly36963 marked this pull request as ready for review October 5, 2023 03:40
@jly36963 jly36963 changed the title feat: add startsWith feat: add startsWith and endsWith Oct 5, 2023
@jly36963
Copy link
Contributor Author

jly36963 commented Oct 5, 2023

@jly36963 I just merged the fix for slice and it now accepts negative indexes

False alarm on my part -- negative indices will always return false for endsWith. I didn't have to worry about forwarding negative indices to Slice

The behavior for out-of-bounds indices and negative indices are opposite for startsWith/endsWith. You'll notice this if you look at the tests for each.

Copy link
Owner

@gustavoguichard gustavoguichard left a comment

Choose a reason for hiding this comment

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

LGTM

@gustavoguichard gustavoguichard merged commit d76bc8e into main Oct 5, 2023
1 check passed
@gustavoguichard gustavoguichard deleted the add-startsWith branch October 5, 2023 13:13
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.

None yet

2 participants