-
Notifications
You must be signed in to change notification settings - Fork 246
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
Refactor/waku.doc #1950
Refactor/waku.doc #1950
Conversation
Moved out all code from docs.go, placed much of the seemingly misc code into much more closely related files, created a dedicated const file for housing all the package consts.
Pull Request Checklist
|
Jenkins BuildsClick to see older builds (6)
|
if err != nil { | ||
t.Fatalf("failed Wrap with seed %d: %s.", seed, 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.
Just so I'm not trying to sneak this through, I want to point out the I've removed this because the condition is always false because err is always nil. The err is tested on line 181 and doesn't change after that check and before the check I've removed.
b, err := rlp.EncodeToBytes(response) | ||
require.NoError(t, err) | ||
|
||
var mresponse MultiVersionResponse | ||
require.NoError(t, rlp.DecodeBytes(b, &mresponse)) |
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've also changed bytes
to b
to remove the type name / variable name collision.
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.
Linter fails, but looks good!
File formatting lint fail. The worst of all lint fails 😭 |
@cammellos codeclimate is complaining also because there is duplication of code between Whisper and Waku. I know we are in the middle of refactoring everything, is it possible to ignore these warnings from codeclimate and address it in a separate PR? |
@Samyoul yes, codeclimate is not required, so they can safely be ignored. |
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.
LGTM, but do we care that whisper/doc.go
has the same issue?
Yes, I care. @jakubgs would it make sense to update this in a new PR? |
Don't see the point of a separate PR. Separate commit is just fine. |
@cammellos , I'm going to restart the tests and see if that makes any kind of difference (it has before in race condition fails). I'm working on the |
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.
Nice.
I also spotted a load of redundant type casting and fixed some of them
Why make the change?
waku/doc.go
is a hangover from https://github.com/ethereum/go-ethereum/blob/master/whisper/whisperv6/doc.go and has become a bit of a dumping ground for any miscellaneous waku functionality or structs.Additionally the golang docs convention reserves the
doc.go
file for housing only docs.What has changed?
doc.go
anddoc_test.go
const.go
EnvelopeError{}
toenvelope.go
mailserver_response.go
tomailserver.go
MailServer{}
interface tomailserver.go
MailServerResponse{}
tomailserver.go
MessagesRequest{}
,MessagesResponse{}
,MultiVersionResponse{}
,Version1MessageResponse{}
andNewMessagesResponse()
tomessage.go
TestEncodeDecodeVersionedResponse{}
tomessage_test.go
RateLimits{}
torate_limiter.go
The same changes were also applied to the
whisper
package.