-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
chore: use errors.New to replace fmt.Errorf with no parameters will much better #5377
base: main
Are you sure you want to change the base?
Conversation
…uch better Signed-off-by: ChengenH <hce19970702@gmail.com>
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.
Isn’t there any linter like errorlint or gosimple that could ensure such best practice ?
It's not serious, so... |
"Heaven is in the details" would say Uncle Bob 😉 |
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.
If you're interested in this clean-up, there are many other places with similar usage:
./model/ids.go:90: return TraceID{}, fmt.Errorf("invalid length for TraceID")
./model/ids.go:97: return nil, fmt.Errorf("unsupported method TraceID.MarshalText; please use github.com/gogo/protobuf/jsonpb for marshalling")
./model/ids.go:102: return fmt.Errorf("unsupported method TraceID.UnmarshalText; please use github.com/gogo/protobuf/jsonpb for marshalling")
./model/ids.go:127: return 0, fmt.Errorf("buffer is too short")
./model/ids.go:184: return SpanID(0), fmt.Errorf("invalid length for SpanID")
./model/ids.go:191: return nil, fmt.Errorf("unsupported method SpanID.MarshalText; please use github.com/gogo/protobuf/jsonpb for marshalling")
./model/ids.go:196: return fmt.Errorf("unsupported method SpanID.UnmarshalText; please use github.com/gogo/protobuf/jsonpb for marshalling")
./pkg/config/tlscfg/options.go:78: return nil, fmt.Errorf("minimum tls version can't be greater than maximum tls version")
./pkg/config/tlscfg/options.go:108: return nil, fmt.Errorf("for client auth via TLS, either both client certificate and key must be supplied, or neither")
./pkg/config/tlscfg/options_test.go:162: return nil, fmt.Errorf("fake system pool")
./pkg/clientcfg/clientcfghttp/thrift-0.9.2/ttypes.go:59: return SamplingStrategyType(0), fmt.Errorf("not a valid SamplingStrategyType string")
./pkg/es/errors_test.go:15: require.ErrorContains(t, fmt.Errorf("some err"), "some err", "no panic")
./pkg/es/config/config.go:366: return nil, fmt.Errorf("both Password and PasswordFilePath are set")
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5377 +/- ##
==========================================
- Coverage 96.19% 96.18% -0.02%
==========================================
Files 327 327
Lines 16012 16012
==========================================
- Hits 15403 15401 -2
- Misses 435 436 +1
- Partials 174 175 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
alternatively, I agree with @mmorel-35 , we should find a linter that catches this, rather than doing manually. I thought revive linter would do that (#5505), but it doesn't. |
## Which problem is this PR solving? - revive is a pretty comprehensive linter that catches many issues - incidentally, I wanted to have a linter for the change in #5377, but the `errorf` [rule in revive](https://github.com/mgechev/revive/blob/master/RULES_DESCRIPTIONS.md#errorf) is only catching `errors.New(fmt.Sprintf())`, not `fmt.Errorf(just_string)` scenarios. ## Description of the changes - add revive to the main list of linters - disable some of its rules which are currently breaking for future clean-up - clean-up a few places what had low number of breaks ## How was this change tested? - `make lint` Signed-off-by: Yuri Shkuro <github@ysh.us>
use errors.New to replace fmt.Errorf with no parameters will much better