apply the megacheck code vetting tool for idiomatic go #3949

zramsay · 2017-05-31T22:13:25Z

megacheck applies each of the unused, gosimple, and staticcheck tools to the code base in order to enforce idiomatic golang. See here for more information.
Only the least controversial recommendations were applied, e.g., some unused functions were left as they may have an intended use. Unused variables were commented out to reduce the number of recommendations provided by the tool. Other recommended changes I skipped because the concepts are new to me.

Main Changes

error handling: if err != nil { return err }; return nil was simplified to return err
bool checks: if pin == false is the same as if !pin
replaced the deprecated os.SEEK_* with io.Seek*
specifying the capacity in a make() was dropped if set to the same as its length.
replaced time.Now().Sub with time.Since()
for loops that could be replaced with append(a, b...), were

License: MIT Signed-off-by: Zach Ramsay <zach.ramsay@gmail.com>

zramsay · 2017-06-01T01:45:28Z

the failure on codeclimate is a false positive (calling context.TODO() is valid and not equal to // TODO). I've submitted a ticket to them.

whyrusleeping · 2017-06-01T01:51:57Z

cmd/ipfs/main.go

-	errApiVersionMismatch  = errors.New("api version mismatch")
-	errRequestCanceled     = errors.New("request canceled")
+	// errUnexpectedApiOutput = errors.New("api returned unexpected output")
+	// errApiVersionMismatch  = errors.New("api version mismatch")


if its actually not being used, should probably just remove it

whyrusleeping · 2017-06-01T01:52:37Z

cmd/ipfswatch/main.go

@@ -99,7 +100,7 @@ func run(ipfsPath, watchPath string) error {
 	}

 	interrupts := make(chan os.Signal)
-	signal.Notify(interrupts, os.Interrupt, os.Kill)
+	signal.Notify(interrupts, os.Interrupt, syscall.SIGTERM)


is os.Kill deprecated?

the error message is: os.Kill cannot be trapped (did you mean syscall.SIGTERM?) (SA1016) where SA1016 -> Trapping a signal that cannot be trapped

whyrusleeping · 2017-06-01T01:57:29Z

routing/mock/centralized_test.go

@@ -45,7 +45,7 @@ func TestClientFindProviders(t *testing.T) {
 	providersFromClient := client.FindProvidersAsync(context.Background(), k, max)
 	isInClient := false
 	for pi := range providersFromClient {
-		if pi.ID == pi.ID {
+		if pi.ID == pi.ID { // <-- typo?


this code is so old and unused... We can probably just remove routing/mock and the supernode routing stuff in general (as a separate PR)

whyrusleeping

This looks great, thanks!

Could you go ahead and just delete the things you commented out in a separate commit (so i don't have to re-read the entire diff again)

whyrusleeping · 2017-06-01T02:01:48Z


nvm, read your comment

zramsay · 2017-06-01T02:20:02Z

sounds good, will address. What value is the codeclimate integration providing over and above implementing this TODO ? IMO the integration should be dropped in favour of implementing that as part of make test (see last line on original PR comment). Happy to pick that up as a seperate PR

License: MIT Signed-off-by: Zach Ramsay <zach.ramsay@gmail.com>

Kubuxu · 2017-06-01T17:34:26Z

Rules.mk

+	@go get honnef.co/go/tools/cmd/megacheck
+	@for pkg in ${PACKAGES_NOVENDOR}; do megacheck "$$pkg"; done
+
+.PHONY: megacheck


Please add this target to mk/golang.mk instead and add comment about it to help print section.

Kubuxu · 2017-06-01T17:40:26Z

unixfs/hamt/hamt_stress_test.go

@@ -189,104 +186,3 @@ func genOpSet(seed int64, keep, temp []string) []testOp {
 		}
 	}
 }
-
-// executes the given op set with a repl to allow easier debugging
-func debugExecuteOpSet(ds dag.DAGService, width int, ops []testOp) (*HamtShard, error) {


This probably shouldn't be removed. It could be useful hot-plug debugging tool.

cc @whyrusleeping

reinstated it but commented it out

because context.TODO() is a legit function License: MIT Signed-off-by: Zach Ramsay <zach.ramsay@gmail.com>

License: MIT Signed-off-by: Zach Ramsay <zach.ramsay@gmail.com>

zramsay · 2017-06-01T18:29:58Z

circle will probably pass after re-triggered. I've notice some non-determinism with the tests (e.g., jenkins failed randomly on a commit where the only change was to .codeclimate.yml

Kubuxu · 2017-06-01T19:39:28Z

Test are deterministic just build machines of free service providers sometimes hang on our network tests.

whyrusleeping · 2017-06-03T00:10:40Z

core/commands/files/files.go

@@ -664,11 +664,6 @@ stat' on the file or any of its ancestors.
 			return
 		}

-		var r io.Reader = input
-		if countfound {
-			r = io.LimitReader(r, int64(count))


Hrm... Maybe you discovered a bug here. I think the copy below was supposed to be from 'r' instead of 'input'

makes sense. The tests are happy with the fix.

License: MIT Signed-off-by: Zach Ramsay <zach.ramsay@gmail.com>

whyrusleeping · 2017-06-07T02:54:09Z

@zramsay Thanks a bunch! This is really useful :) If there are more, feel free to file more PRs

zramsay · 2017-06-07T14:49:07Z

no problem! There a couple dozen ~more complex fixes that I'll tackle as part of #3953

zramsay added 2 commits May 31, 2017 16:40

introduce 'make megacheck'

f9ba45d

License: MIT Signed-off-by: Zach Ramsay <zach.ramsay@gmail.com>

apply the megacheck tool to improve code quality

c5df8f0

License: MIT Signed-off-by: Zach Ramsay <zach.ramsay@gmail.com>

whyrusleeping reviewed Jun 1, 2017

View reviewed changes

whyrusleeping requested changes Jun 1, 2017

View reviewed changes

zramsay mentioned this pull request Jun 1, 2017

remove routing/mock and the supernode routing stuff #3950

Closed

address PR comments; remove commented/dead code

096b323

License: MIT Signed-off-by: Zach Ramsay <zach.ramsay@gmail.com>

Kubuxu reviewed Jun 1, 2017

View reviewed changes

zramsay mentioned this pull request Jun 1, 2017

complete the recomendations from gometalinter and include in the test sequence #3953

Closed

Kubuxu reviewed Jun 1, 2017

View reviewed changes

zramsay added 3 commits June 1, 2017 14:04

codeclimate: use only 4/5 defaults

0b58458

because context.TODO() is a legit function License: MIT Signed-off-by: Zach Ramsay <zach.ramsay@gmail.com>

make: use correct location for [test_go_megacheck]

ae4cad3

License: MIT Signed-off-by: Zach Ramsay <zach.ramsay@gmail.com>

hamt: reinstate a useful debug function

b77d5aa

License: MIT Signed-off-by: Zach Ramsay <zach.ramsay@gmail.com>

whyrusleeping reviewed Jun 3, 2017

View reviewed changes

bugfix: copy the correct variable

6c064d1

License: MIT Signed-off-by: Zach Ramsay <zach.ramsay@gmail.com>

whyrusleeping approved these changes Jun 7, 2017

View reviewed changes

whyrusleeping merged commit eef022c into ipfs:master Jun 7, 2017

zramsay deleted the fix/codebase-consistency branch June 7, 2017 14:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

apply the megacheck code vetting tool for idiomatic go #3949

apply the megacheck code vetting tool for idiomatic go #3949

zramsay commented May 31, 2017 •

edited

zramsay commented Jun 1, 2017 •

edited

whyrusleeping Jun 1, 2017

zramsay Jun 1, 2017

whyrusleeping Jun 1, 2017

zramsay Jun 1, 2017

whyrusleeping Jun 1, 2017

zramsay Jun 1, 2017

whyrusleeping left a comment

whyrusleeping commented Jun 1, 2017 •

edited

zramsay commented Jun 1, 2017 •

edited

Kubuxu Jun 1, 2017

zramsay Jun 1, 2017

Kubuxu Jun 1, 2017

zramsay Jun 1, 2017

zramsay commented Jun 1, 2017

Kubuxu commented Jun 1, 2017

whyrusleeping Jun 3, 2017

zramsay Jun 3, 2017

whyrusleeping commented Jun 7, 2017

zramsay commented Jun 7, 2017

apply the megacheck code vetting tool for idiomatic go #3949

apply the megacheck code vetting tool for idiomatic go #3949

Conversation

zramsay commented May 31, 2017 • edited

Main Changes

zramsay commented Jun 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whyrusleeping left a comment

Choose a reason for hiding this comment

whyrusleeping commented Jun 1, 2017 • edited

zramsay commented Jun 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zramsay commented Jun 1, 2017

Kubuxu commented Jun 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whyrusleeping commented Jun 7, 2017

zramsay commented Jun 7, 2017

zramsay commented May 31, 2017 •

edited

zramsay commented Jun 1, 2017 •

edited

whyrusleeping commented Jun 1, 2017 •

edited

zramsay commented Jun 1, 2017 •

edited