-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Use strings.Cut and strings.CutPrefix where possible #4470
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
Changes from all commits
7149781
871d918
bfcd479
4271ecf
075cea3
037668e
40ce69c
930cd49
e9855bd
f134871
d83d533
ef983f5
ecf7430
259b71c
055041e
746a5c2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,29 +58,29 @@ func statPSI(dirPath string, file string) (*cgroups.PSIStats, error) { | |
func parsePSIData(psi []string) (cgroups.PSIData, error) { | ||
data := cgroups.PSIData{} | ||
for _, f := range psi { | ||
kv := strings.SplitN(f, "=", 2) | ||
if len(kv) != 2 { | ||
key, val, ok := strings.Cut(f, "=") | ||
if !ok { | ||
return data, fmt.Errorf("invalid psi data: %q", f) | ||
} | ||
var pv *float64 | ||
switch kv[0] { | ||
switch key { | ||
case "avg10": | ||
pv = &data.Avg10 | ||
case "avg60": | ||
pv = &data.Avg60 | ||
case "avg300": | ||
pv = &data.Avg300 | ||
case "total": | ||
v, err := strconv.ParseUint(kv[1], 10, 64) | ||
v, err := strconv.ParseUint(val, 10, 64) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not for this PR, but I started to look in some other codebases to dismantle these errors. The default error returned is something like; invalid total PSI value: strconv.ParseUint: parsing "some invalid value": invalid syntax Which tends to be a bit "implementation detail", and https://go.dev/play/p/E9Dx1YBzPiN var numErr *strconv.NumError
if errors.As(err, &numErr) {
fmt.Printf("invalid %s PS value (%s): %w", key, numErr.Num, numErr.Err)
} Which would be something like; invalid total PSI value (some invalid value): invalid syntax There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Indeed it isn't, but at least it says which function returned the error, and any developer (and some advanced users) will deduce that an unsigned integer was expected. Generally speaking, to make this error more user-friendly, the error text should say something like "expected unsigned integer value, got %q: %w", and the best place to fix this is the actual In this case, though, we're parsing kernel-generated data (and we actually check that it comes from the kernel, by checking the filesystem type in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, it really was just me rambling a bit, having dealt too many times with users "I have the same error" (because the Probably something for a rainy afternoon to look if I can come with some nice approach for these. 😄 |
||
if err != nil { | ||
return data, fmt.Errorf("invalid %s PSI value: %w", kv[0], err) | ||
return data, fmt.Errorf("invalid %s PSI value: %w", key, err) | ||
} | ||
data.Total = v | ||
} | ||
if pv != nil { | ||
v, err := strconv.ParseFloat(kv[1], 64) | ||
v, err := strconv.ParseFloat(val, 64) | ||
if err != nil { | ||
return data, fmt.Errorf("invalid %s PSI value: %w", kv[0], err) | ||
return data, fmt.Errorf("invalid %s PSI value: %w", key, err) | ||
} | ||
*pv = v | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I havn't checked that whether this will lead to some regressions or not, for example, if c == "a:b:c:d:e":
When using
SplitN
,cs[1]
will be "b";When using
Cut
,path
will be "b:c:d:e".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, the old code would error out when
c == "a:b:c:d:e"
.The new code would not, assigning
b:c:d:e
topath
.To me, the new code is (ever so slightly) more correct, since
:
is a valid symbol which should be allowed inpath
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, was looking at that as well; agreed, looks like new code is more correct here.
I still need to have a quick look at the splitting on commas, and the
if len(args) != 1 {
check below. Probably will check out this branch locally