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

refactor: replace strings.Replace with strings.ReplaceAll #7660

Merged
merged 2 commits into from Jun 1, 2022
Merged

refactor: replace strings.Replace with strings.ReplaceAll #7660

merged 2 commits into from Jun 1, 2022

Conversation

Juneezee
Copy link
Contributor

Replace strings.Replace(s, old, new, -1) with strings.ReplaceAll(s, old, new). strings.ReplaceAll is a wrapper function for strings.Replace, but strings.ReplaceAll is more readable and removes the hardcoded -1.

@CLAassistant
Copy link

CLAassistant commented May 20, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@albscui albscui left a comment

Choose a reason for hiding this comment

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

LGTM. We added the requirement to keep branches up to date with the target branch. You might want to update your branch, then merge as soon as CI passes.

@JoeyZwicker
Copy link
Member

JoeyZwicker commented May 25, 2022

hey team, just checking in on this PR and if there's anything in CI that we need to fix to get this merged in

Juneezee and others added 2 commits May 26, 2022 23:29
strings.ReplaceAll(s, old, new) is a wrapper function for
strings.Replace(s, old, new, -1). But strings.ReplaceAll is more
readable and removes the hardcoded -1.

Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
@PFedak
Copy link
Contributor

PFedak commented Jun 1, 2022

Thanks @Juneezee , I suspect our CI isn't configured properly to run on PRs from forks. I'm just going to override the checks and merge.

@PFedak PFedak merged commit 46e73c1 into pachyderm:master Jun 1, 2022
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

5 participants