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.

Sign up for GitHub

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

Jump to bottom

Add findfirst, findlast for searching char in strings #31664

Merged
merged 17 commits into from
Jun 3, 2019
Merged

Add findfirst, findlast for searching char in strings #31664

merged 17 commits into from
Jun 3, 2019

Conversation

eulerkochy
Copy link
Contributor

Addresses #31600

nalimilan
nalimilan reviewed Apr 10, 2019
View reviewed changes
Copy link
Member

@nalimilan nalimilan left a comment

Choose a reason for hiding this comment

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

Thanks!

base/strings/search.jl Outdated Show resolved Hide resolved
test/strings/search.jl Outdated Show resolved Hide resolved
test/strings/search.jl Outdated Show resolved Hide resolved
@nalimilan
Copy link
Member

Ah, and maybe saying that it's equivalent to findfirst(==(ch), string) would be more natural?

It would also make sense to support findnext and findprev.

@eulerkochy
Copy link
Contributor Author

@nalimilan any more work on this PR?

@fredrikekre fredrikekre added needs compat annotation Add !!! compat "Julia x.y" to the docstring needs news A NEWS entry is required for this change labels Apr 11, 2019
nalimilan
nalimilan reviewed Apr 12, 2019
View reviewed changes
base/strings/search.jl Outdated Show resolved Hide resolved
base/strings/search.jl Outdated Show resolved Hide resolved
test/strings/search.jl Outdated Show resolved Hide resolved
nalimilan
nalimilan reviewed Apr 12, 2019
View reviewed changes
Copy link
Member

@nalimilan nalimilan left a comment

Choose a reason for hiding this comment

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

Do you feel like implementing findnext and findprev? It shouldn't be a lot of work and it would be more consistent.

base/strings/search.jl Show resolved Hide resolved
@eulerkochy
Copy link
Contributor Author

@nalimilan is this alright now?

nalimilan
nalimilan reviewed Apr 15, 2019
View reviewed changes
base/strings/search.jl Outdated Show resolved Hide resolved
base/strings/search.jl Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
test/strings/search.jl Outdated Show resolved Hide resolved
@nalimilan nalimilan removed needs compat annotation Add !!! compat "Julia x.y" to the docstring needs news A NEWS entry is required for this change labels Apr 15, 2019
@eulerkochy
Copy link
Contributor Author

@nalimilan all bases covered?

@fredrikekre fredrikekre added search & find The find* family of functions strings "Strings!" labels Apr 16, 2019
nalimilan
nalimilan approved these changes Apr 16, 2019
View reviewed changes
Copy link
Member

@nalimilan nalimilan left a comment

Choose a reason for hiding this comment

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

Thanks! I've added a commit to use AbstractChar instead of Char.

@eulerkochy
Copy link
Contributor Author

Bump

@StefanKarpinski
Copy link
Member

I attempted to commit some changes to the branch but it doesn't seem to allow maintainers to make commits. The compat notes should say that the method is new, not the function—the function already existed, it's the methods that are new.

eulerkochy and others added 2 commits May 29, 2019 18:54
@eulerkochy @StefanKarpinski
Update base/strings/search.jl
19c352f 
Co-Authored-By: Stefan Karpinski <[email protected]>
@eulerkochy @StefanKarpinski
Apply suggestions from code review
9a635dc 
Co-Authored-By: Stefan Karpinski <[email protected]>
@eulerkochy
Copy link
Contributor Author

Is it alright now @StefanKarpinski ?

@StefanKarpinski StefanKarpinski merged commit d249e71 into JuliaLang:master Jun 3, 2019
@sostock sostock mentioned this pull request Jul 9, 2021
Closed
@jyjemily jyjemily mentioned this pull request May 29, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StefanKarpinski StefanKarpinski StefanKarpinski left review comments

@nalimilan nalimilan nalimilan approved these changes

Assignees
No one assigned
Labels
search & find The find* family of functions strings "Strings!"
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@eulerkochy @nalimilan @StefanKarpinski @fredrikekre