search

libp2p / go-libp2p-pubsub

The PubSub implementation for go-libp2p
https://github.com/libp2p/specs/tree/master/pubsub
Other
317 stars 182 forks source link

cleanup: fix vet and staticcheck failures #435

Closed iand closed 3 years ago

iand commented 3 years ago

Fixes the safe staticcheck failures. The following remain and need more thought or explicit ignores.

May indicate a bug, not clear what the intention is here:

gossipsub.go:904:4: ineffective break statement. Did you mean to break out of the outer loop? (SA4011)

Needs a decision on the alternative:

gossipsub.go:1300:8: log.EventBegin is deprecated: Stop using go-log for event logging  (SA1019)

Changing these may break clients who rely on error string text:

validation.go:146:15: error strings should not be capitalized (ST1005)
validation.go:173:15: error strings should not be capitalized (ST1005)
validation.go:209:15: error strings should not be capitalized (ST1005)
validation.go:387:9: error strings should not be capitalized (ST1005)
vyzo commented 3 years ago

gossipsub.go:904:4: ineffective break statement

That thing with the break is really innocuous, we can just remove it.

gossipsub.go:1300:8: log.EventBegin is deprecated: Stop using go-log for event logging (SA1019)

We can just replace it with a log.Infow that logs the timing

Changing these may break clients who rely on error string text:

That's just silly/stupid; just go ahead and change them.

iand commented 3 years ago

breaking

Is this a reference to the replacement of t.Fatal with t.Error+return in test goroutines? In these cases the test still fails.

vyzo commented 3 years ago

yeah, just worried it might get blocked somewhere.

On Mon, Jul 19, 2021, 15:55 Ian Davis @.***> wrote:

breaking

Is this a reference to the replacement of t.Fatal with t.Error+return in test goroutines? In these cases the test still fails.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/libp2p/go-libp2p-pubsub/pull/435#issuecomment-882523279, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAI4STAYF6ZDBKPD2PIAN3TYQOCXANCNFSM5ATQVWAA .

iand commented 3 years ago

yeah, just worried it might get blocked somewhere.

OK. In TestGossipsubAttackSpamIHAVE returning from the goroutine cancels the context. I could use that to ensure that the other goroutines exit too (we should do that anyway to avoid other sources of blocking). Will look at the other tests as well.

iand commented 3 years ago

yeah, just worried it might get blocked somewhere.

OK. In TestGossipsubAttackSpamIHAVE returning from the goroutine cancels the context. I could use that to ensure that the other goroutines exit too (we should do that anyway to avoid other sources of blocking). Will look at the other tests as well.

Done this in https://github.com/libp2p/go-libp2p-pubsub/pull/435/commits/4238ca7c078c6c632641a9730aea9e1d689ba637