-
Notifications
You must be signed in to change notification settings - Fork 662
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
Update golangci-lint
to v1.56.1
#2714
Conversation
golangci-lint
to v1.56.0
golangci-lint
to v1.56.1
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.
sick 🔥 🚀
@@ -300,13 +300,13 @@ func (vtx *uniqueVertex) String() string { | |||
len(txs), | |||
)) | |||
|
|||
parentFormat := fmt.Sprintf("\n Parent[%s]: ID = %%s, Status = %%s", | |||
parentFormat := fmt.Sprintf("\n Parent[%s]: ID = %%s, Status = %%s", //nolint:perfsprint |
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.
why are we disabling linting here? There were other places where we were able to replace Sprintf with simpler alternative. What's the difference here?
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 tried changing it a few times but the code got more confusing. We don't stringify the unique vertex enough to sacrifice code clarity for performance IMO
keys := make([]*secp256k1.PrivateKey, 5) | ||
for i := range keys { | ||
key, err := secp256k1.NewPrivateKey() | ||
require.NoError(err) | ||
require.NoError(t, err) |
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.
why? Is there a linter complaining about this?
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.
When we use require, we should use it with the most scoped t
variable. Further down this test, we do require := require.New(t)
in the sub-test. The linter was complaining that the sub-test t *testing.T
was unused.
@@ -21,7 +21,7 @@ type noOpTracer struct { | |||
} | |||
|
|||
func (n noOpTracer) Start(ctx context.Context, spanName string, opts ...trace.SpanStartOption) (context.Context, trace.Span) { | |||
return n.t.Start(ctx, spanName, opts...) | |||
return n.t.Start(ctx, spanName, opts...) //nolint:spancheck |
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.
the linter suggests there is a possible memory leak here. Should we end the spam right away for the noop tracer instead of silencing the linter?
NoOpTracer does not seem to be used in prod code currently but I think it's best to avoid any possible memory leak, even in UTs
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.
The consumer should be ending the span. The linter is complaining that the returned span is unassigned so there could be a possible memory leak
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.
just a couple of points where we may avoid silencing the linter. All in all nice cleanup!
638d385
to
013f4b1
Compare
Why this should be merged
Linters 🧹
How this works
gci
and removesgoimports
spancheck
testifylint
and removes some of our bash lintersunused-parameter
to check anonymous functions mgechev/revive#965 (filed by @StephenButtolph ;))How this was tested
CI