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

Replace strings.Index usages with strings.Cut #4930

Merged
merged 2 commits into from
Aug 4, 2022

Conversation

WilczynskiT
Copy link
Contributor

@WilczynskiT WilczynskiT commented Aug 4, 2022

fixed #4928

@CLAassistant
Copy link

CLAassistant commented Aug 4, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Thank you for opening a PR!

I think the purpose of using strings.Cut() is to split a string into two pieces. strings.Index() is still the right function to see if a substring can be found.

There are some uses of Index() that still seem simpler than using Cut(), IMO.

I think we'd only want to keep the changes in listeners.go, load.go, and addresses.go. The others seem to be more complicated or not quite the right fit.

There may also be some occurrences of SplitN() that we could replace with Cut(), if we are splitting only into 2 pieces.

@WilczynskiT
Copy link
Contributor Author

@mholt Okey, thank you for review ;) I changed it. I will look at the SplitN() and will create new ticket to do it

@francislavoie
Copy link
Member

francislavoie commented Aug 4, 2022

I will look at the SplitN() and will create new ticket to do it

Please just do it in this PR, it'll be easier to review all in one place.

Also, please remember to sign the CLA.

To clarify, the intent of the issue I opened is to find places where we can use the new strings.Cut to simplify existing code. If some places have legitimate uses of the existing strings.* functions that are already simple, then there's no need to change that code.

@mholt mholt added the under review 🧐 Review is pending before merging label Aug 4, 2022
@mholt mholt added this to the v2.5.3 milestone Aug 4, 2022
@WilczynskiT
Copy link
Contributor Author

Okey, I added the changes and signed the CLA. So it is ready to continue the review.

@WilczynskiT WilczynskiT requested a review from mholt August 4, 2022 09:16
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

This LGTM!

Thank you for the contribution @WilczynskiT 😃

@mholt mholt added optimization 📉 Performance or cost improvements and removed under review 🧐 Review is pending before merging labels Aug 4, 2022
@mholt mholt merged commit 2642bd7 into caddyserver:master Aug 4, 2022
@WilczynskiT WilczynskiT deleted the use-strings-cut branch August 16, 2022 21:23
WilczynskiT added a commit to WilczynskiT/caddy that referenced this pull request Aug 17, 2022
@mholt mholt modified the milestones: v2.6.0-beta.1, v2.6.0 Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
optimization 📉 Performance or cost improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace strings.Index usages with strings.Cut where possible
4 participants