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

Add text_position #505

Open
wants to merge 2 commits into
base: draft
Choose a base branch
from
Open

Add text_position #505

wants to merge 2 commits into from

Conversation

m-mohr
Copy link
Member

@m-mohr m-mohr commented May 17, 2024

No description provided.

@soxofaan
Copy link
Member

This process is pretty similar to array_find, so it might be good to align with that one:

  • text_find instead of test_position
  • array_find returns null when not found instead of -1
  • rename pattern arg to value too?

@m-mohr
Copy link
Member Author

m-mohr commented May 29, 2024

Good point.

text_find instead of test_position

Sounds good. +1

array_find returns null when not found instead of -1

I don't have strong preferences here. +0

rename pattern arg to value too?

pattern is coming from text_contains, text_begins, ...
I think having it consistent in text_* is more important than having it consistent with array_find. -1

Edit: Updated the PR accordingly.

@soxofaan
Copy link
Member

pattern is coming from text_contains, text_begins, ...

good point

Another thing that is worth mentioning is that the returned position is a zero-based index (as mentioned in array_find)

@m-mohr
Copy link
Member Author

m-mohr commented May 30, 2024

Isn't that captured by the description of the return value?

A value >= 0 that indicates the position of the text

If not, happy to accept proposals how to improve.

@soxofaan
Copy link
Member

soxofaan commented Jun 3, 2024

I just mentioned it because I noticed it's the first statement in the description of array_find

Returns the zero-based index of ...

while it's not explicitly mentioned in current PR of text_find

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants