Skip to content
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

*: fix govet-shadow lint #16608

Merged
merged 1 commit into from
Sep 19, 2023
Merged

*: fix govet-shadow lint #16608

merged 1 commit into from
Sep 19, 2023

Conversation

fuweid
Copy link
Member

@fuweid fuweid commented Sep 18, 2023

@@ -181,33 +181,33 @@ func (t *Trace) logInfo(threshold time.Duration) (string, []zap.Field) {
var steps []string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer needed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, Line 207 is still using it~

Copy link
Member

@serathius serathius Sep 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True

step, steps, tstep, t.steps.

Think this function needs a for major redo.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use tstep here based on t.steps. I mark it as TODO if need.

Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Thanks @fuweid

@ahrtr
Copy link
Member

ahrtr commented Sep 19, 2023

@fuweid I just merged a huge PR #16595, could you please rebase this PR although github doesn't show conflict? thx

if err := b.ForEach(func(k, v []byte) error {
if _, err := h.Write(k); err != nil {
if err = b.ForEach(func(k, v []byte) error {
if _, err = h.Write(k); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The err (line 152) overwrites/updates the err outside closure (line 151) directly? I know it works, but it looks a little strange. Can we rename it, e.g. berr? Same below

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resp, err := cc.Get(ctx, "/")
if err != nil {
return err
resp, gerr := cc.Get(ctx, "/")
Copy link
Member

@ahrtr ahrtr Sep 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The renaming looks good to me. But why do you follow a different approach in other places? For example,

  • in tests/robustness/traffic/traffic.go, you define var c *RecordingClient in order to reuse the err variable.
  • tools/etcd-dump-logs/main.go, it's even worse. You overwrites/updates the err defined outside the switch statement.
  • lots of other similar cases.

Usually simpler is better. In this example, renaming the err variable in a small scope (such as here gerr) is better than defining a new resp variable. So I suggest to follow the same approach for all other places as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated with some comments. PTAL.

if err != nil {
m.logAndCloseWithError(err, pw)
return
}
for {
resp, err := ss.Recv()
sresp, err := ss.Recv()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use new sresp here because the old name shadows the L240.

./maintenance.go:253:4: declaration of "resp" shadows declaration at line 240

ctx, cancel := context.WithTimeout(context.Background(), s.Cfg.ReqTimeout())
resp, lastErr := HashByRev(ctx, s.cluster.ID(), cc, ep, rev)
resp, lastErr = HashByRev(ctx, s.cluster.ID(), cc, ep, rev)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

create new var to prevent shadow the lastErr.

etcdserver/corrupt.go:481:10: declaration of "lastErr" shadows declaration at line 478

@@ -49,7 +49,9 @@ func NewTmpWAL(t testing.TB, reqs []etcdserverpb.InternalRaftRequest) (*wal.WAL,
if err != nil {
t.Fatalf("Failed to open WAL: %v", err)
}
_, state, _, err := w.ReadAll()

var state raftpb.HardState
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating new var state to reduce the line changes. err doesn't reuse in other goroutines. using err here might be more easy to review.

etcdutl/snapshot/v3_snapshot.go Outdated Show resolved Hide resolved
etcdutl/snapshot/v3_snapshot.go Outdated Show resolved Hide resolved
Signed-off-by: Wei Fu <fuweid89@gmail.com>
Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

thx @fuweid

@ahrtr ahrtr merged commit 36403e3 into etcd-io:main Sep 19, 2023
27 checks passed
@fuweid fuweid deleted the fix-shadow-vet branch September 19, 2023 14:29
@jmhbnz jmhbnz mentioned this pull request Sep 25, 2023
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants