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

apply the megacheck code vetting tool for idiomatic go #3949

Merged
merged 7 commits into from
Jun 7, 2017
Merged

apply the megacheck code vetting tool for idiomatic go #3949

merged 7 commits into from
Jun 7, 2017

Conversation

zramsay
Copy link
Contributor

@zramsay zramsay commented May 31, 2017

  • 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>
License: MIT
Signed-off-by: Zach Ramsay <zach.ramsay@gmail.com>
@zramsay
Copy link
Contributor Author

zramsay commented Jun 1, 2017

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.

cmd/ipfs/main.go Outdated
errApiVersionMismatch = errors.New("api version mismatch")
errRequestCanceled = errors.New("request canceled")
// errUnexpectedApiOutput = errors.New("api returned unexpected output")
// errApiVersionMismatch = errors.New("api version mismatch")
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

roger.

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

is os.Kill deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@@ -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?
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

whyrusleeping commented Jun 1, 2017


nvm, read your comment

@zramsay
Copy link
Contributor Author

zramsay commented Jun 1, 2017

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>
Rules.mk Outdated
@go get honnef.co/go/tools/cmd/megacheck
@for pkg in ${PACKAGES_NOVENDOR}; do megacheck "$$pkg"; done

.PHONY: megacheck
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

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

cc @whyrusleeping

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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>
License: MIT
Signed-off-by: Zach Ramsay <zach.ramsay@gmail.com>
@zramsay
Copy link
Contributor Author

zramsay commented Jun 1, 2017

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
Copy link
Member

Kubuxu commented Jun 1, 2017

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

@@ -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))
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense. The tests are happy with the fix.

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

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

@whyrusleeping whyrusleeping merged commit eef022c into ipfs:master Jun 7, 2017
@zramsay
Copy link
Contributor Author

zramsay commented Jun 7, 2017

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

@zramsay zramsay deleted the fix/codebase-consistency branch June 7, 2017 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants