Refactor/waku.doc #1950

Samyoul · 2020-04-23T14:05:11Z

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.

another convention for packages that need large amounts of introductory documentation: the package comment is placed in its own file, doc.go, which contains only those comments and a package clause.

What has changed?

Removal of doc.go and doc_test.go
Moved package consts to const.go
Moved EnvelopeError{} to envelope.go
Renamed mailserver_response.go to mailserver.go
Moved MailServer{} interface to mailserver.go
Moved MailServerResponse{} to mailserver.go
Moved MessagesRequest{}, MessagesResponse{}, MultiVersionResponse{}, Version1MessageResponse{} and NewMessagesResponse() to message.go
Moved TestEncodeDecodeVersionedResponse{} to message_test.go
Moved RateLimits{} to rate_limiter.go

The same changes were also applied to the whisper package.

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.

status-github-bot · 2020-04-23T14:05:17Z

Pull Request Checklist

Have you updated the documentation, if impacted (e.g. docs.status.im)?

status-im-auto · 2020-04-23T14:06:17Z

Jenkins Builds

Click to see older builds (6)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`1a5baa5`	#1	2020-04-23 14:06:17	~52 sec	`linux`	📦`zip`
✔️	`1a5baa5`	#1	2020-04-23 14:11:07	~5 min	`ios`	📦`zip`
✔️	`1a5baa5`	#1	2020-04-23 14:14:14	~8 min	`android`	📦`aar`

✔️	`ab45dbb`	#2	2020-04-23 15:15:32	~46 sec	`linux`	📦`zip`
✔️	`ab45dbb`	#2	2020-04-23 15:18:29	~3 min	`ios`	📦`zip`
✔️	`ab45dbb`	#2	2020-04-23 15:21:37	~6 min	`android`	📦`aar`

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`04413b9`	#3	2020-04-23 16:18:12	~41 sec	`linux`	📦`zip`
✔️	`04413b9`	#3	2020-04-23 16:22:58	~5 min	`ios`	📦`zip`
✔️	`04413b9`	#3	2020-04-23 16:25:59	~8 min	`android`	📦`aar`

✔️	`2680f60`	#4	2020-04-23 16:46:45	~39 sec	`linux`	📦`zip`
✔️	`2680f60`	#4	2020-04-23 16:48:53	~2 min	`ios`	📦`zip`
✔️	`2680f60`	#4	2020-04-23 16:52:38	~6 min	`android`	📦`aar`

Samyoul · 2020-04-23T14:10:00Z

waku/message_test.go

-	if err != nil {
-		t.Fatalf("failed Wrap with seed %d: %s.", seed, err)
-	}


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.

Samyoul · 2020-04-23T14:10:58Z

waku/message_test.go

+	b, err := rlp.EncodeToBytes(response)
+	require.NoError(t, err)
+
+	var mresponse MultiVersionResponse
+	require.NoError(t, rlp.DecodeBytes(b, &mresponse))


I've also changed bytes to b to remove the type name / variable name collision.

cammellos

Linter fails, but looks good!

Samyoul · 2020-04-23T14:17:26Z

File formatting lint fail. The worst of all lint fails 😭

waku/message_test.go

Samyoul · 2020-04-23T14:34:11Z

@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?

cammellos · 2020-04-23T14:38:28Z

@Samyoul yes, codeclimate is not required, so they can safely be ignored.
I tried goimports -w message_test.go but did not show any diff, maybe some difference in versions?

jakubgs

LGTM, but do we care that whisper/doc.go has the same issue?

Samyoul · 2020-04-23T14:47:55Z

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?

jakubgs · 2020-04-23T14:48:57Z

Don't see the point of a separate PR. Separate commit is just fine.

Samyoul · 2020-04-23T14:58:56Z

@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 whisper/doc.go refactor, once done I'll commit and see if there's any change to the mood the linter is in 😆 .

jakubgs

Nice.

I also spotted a load of redundant type casting and fixed some of them

Samyoul added 4 commits April 23, 2020 14:39

Refactor of waku.doc.go

51d2b7a

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.

Moved doc_test to message_test

93425cb

Refactored type collision and check on nil err

92fcfc3

message_test.go fmt

1a5baa5

Samyoul assigned cammellos and jakubgs Apr 23, 2020

Samyoul commented Apr 23, 2020

View reviewed changes

cammellos requested review from cammellos and jakubgs April 23, 2020 14:12

cammellos assigned Samyoul and unassigned cammellos and jakubgs Apr 23, 2020

cammellos approved these changes Apr 23, 2020

View reviewed changes

Samyoul commented Apr 23, 2020

View reviewed changes

waku/message_test.go Show resolved Hide resolved

jakubgs approved these changes Apr 23, 2020

View reviewed changes

Implemented whisper/doc.go refactor

ab45dbb

jakubgs approved these changes Apr 23, 2020

View reviewed changes

Samyoul added 2 commits April 23, 2020 17:17

import order change to attempt to please the linter.

04413b9

Bug fix on whisper_test.go

2680f60

I also spotted a load of redundant type casting and fixed some of them

Samyoul merged commit cc8d577 into develop Apr 23, 2020

delete-merged-branch bot deleted the refactor/waku.doc branch April 23, 2020 19:54

weekly-digest bot mentioned this pull request Apr 26, 2020

Weekly Digest (19 April, 2020 - 26 April, 2020) #1952

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor/waku.doc #1950

Refactor/waku.doc #1950

Samyoul commented Apr 23, 2020 •

edited

status-github-bot bot commented Apr 23, 2020 •

edited by Samyoul

status-im-auto commented Apr 23, 2020 •

edited

Samyoul Apr 23, 2020 •

edited

Samyoul Apr 23, 2020

cammellos left a comment

Samyoul commented Apr 23, 2020

Samyoul commented Apr 23, 2020

cammellos commented Apr 23, 2020

jakubgs left a comment

Samyoul commented Apr 23, 2020

jakubgs commented Apr 23, 2020

Samyoul commented Apr 23, 2020

jakubgs left a comment

Refactor/waku.doc #1950

Refactor/waku.doc #1950

Conversation

Samyoul commented Apr 23, 2020 • edited

Why make the change?

What has changed?

status-github-bot bot commented Apr 23, 2020 • edited by Samyoul

Pull Request Checklist

status-im-auto commented Apr 23, 2020 • edited

Jenkins Builds

Samyoul Apr 23, 2020 • edited

Choose a reason for hiding this comment

Samyoul Apr 23, 2020

Choose a reason for hiding this comment

cammellos left a comment

Choose a reason for hiding this comment

Samyoul commented Apr 23, 2020

Samyoul commented Apr 23, 2020

cammellos commented Apr 23, 2020

jakubgs left a comment

Choose a reason for hiding this comment

Samyoul commented Apr 23, 2020

jakubgs commented Apr 23, 2020

Samyoul commented Apr 23, 2020

jakubgs left a comment

Choose a reason for hiding this comment

Samyoul commented Apr 23, 2020 •

edited

status-github-bot bot commented Apr 23, 2020 •

edited by Samyoul

status-im-auto commented Apr 23, 2020 •

edited

Samyoul Apr 23, 2020 •

edited