Replace strings.SplitN with strings.Cut #21185
Conversation
github-actions
bot
added
machine
kind/api-change
|
@rhatdan did something similar |
| if len(split) != 2 { | ||
| return toReturn, nil, nil, errors.New("must provide a path to a namespace when specifying \"ns:\"") | ||
| } | ||
| _, value, _ := strings.Cut(ns, ":") |
There was a problem hiding this comment.
Changed this code to preserve existing behavior, the split always works because of HasPrefix, but maybe error message was intended for len(value) == 0
| default: | ||
| return false, fmt.Errorf("'U' or 'chown' must be set to true or false, instead received %q: %w", kv[1], errOptionArg) | ||
| } | ||
| func validChownFlag(value string) (bool, error) { |
There was a problem hiding this comment.
Cleaned up this function quite a bit, going as far as not bothering to reparse k=v by changing function signature
|
Ephemeral COPR build failed. @containers/packit-build please check. |
| split := strings.SplitN(label, "=", 2) | ||
| if split[0] == "" { | ||
| key, value, _ := strings.Cut(label, "=") | ||
| if key == "" { |
There was a problem hiding this comment.
Question, but not for addressing in your PR: why is this null-key check not being done everywhere?
|
Test failures look unrelated |
There was a problem hiding this comment.
Still not finished. The validChownFlag() cleanup is lovely, but it took me a long time to understand how it ever worked, because I scrolled linearly and did not see the changed function signature until much later. This is a small block of comments all relating to its unit tests.
There was a problem hiding this comment.
WHEW! First review pass complete. (Breaking with my custom, there will be no second pass).
LGTM modulo my prior feedback. Most are suggestions/questions; the chown test ones are blockers.
Again: really, really nice work. You took time to evaluate each instance, give meaningful variable names, and perform extra cleanup when necessary. Thank you.
Cut is a cleaner & more performant api relative to SplitN(_, _, 2) added in go 1.18 Previously applied this refactoring to buildah: containers/buildah#5239 Signed-off-by: Philip Dubé <philip@peerdb.io>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago, serprex The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/lgtm Thank you, @serprex ! |
github-actions
bot
added
the
locked - please file new issue/PR
Cut is a cleaner & more performant api relative to SplitN(_, _, 2) added in go 1.18
Previously applied this refactoring to buildah: containers/buildah#5239
Does this PR introduce a user-facing change?