chore: use errors.New to replace fmt.Errorf with no parameters will much better #5377
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…uch better Signed-off-by: ChengenH <[email protected]>
ChengenH
changed the title
mmorel-35
approved these changes
Apr 21, 2024
It's not serious, so... |
|
"Heaven is in the details" would say Uncle Bob 😉 |
yurishkuro
approved these changes
Jun 2, 2024
yurishkuro
reviewed
Jun 2, 2024
There was a problem hiding this comment.
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.38% 54.13% -42.25%
===========================================
Files 329 163 -166
Lines 16060 8412 -7648
===========================================
- Hits 15479 4554 -10925
- Misses 404 3414 +3010
- Partials 177 444 +267
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Merged
|
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. |
yurishkuro
added a commit
that referenced
this pull request
Jun 2, 2024
## 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 <[email protected]>
|
no follow-up, closing |
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
use errors.New to replace fmt.Errorf with no parameters will much better