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

tools: Add code complexity linter, gocyclo #1593

Merged
merged 11 commits into from
Jul 9, 2018
Merged

tools: Add code complexity linter, gocyclo #1593

merged 11 commits into from
Jul 9, 2018

Conversation

ValarDragon
Copy link
Contributor

Gocyclo is a code complexity linter. It uses cyclomatic complexity.
Cyclomatic complexity essentially measures the number of different
paths code could go through. (The conditional in a for loop counts
as adding one path) It looks at this on a per-function level. The
idea that this would be enforcing is that if there are too many
different paths code can go through in a function, it needs to be
better split up. (A function with too many code paths is hard to
reason about)

The complexity which we want the linter to start failing on is
configurable. The default is 10. Change the "Cyclo" parameter in
tools/gometalinter.json to try other values.

I've left the errors in this first commit, just so everyone can take a look. (Plus
its likely I don't understand most of the functions well enough to effectively
split them into smaller functions)

  • Updated all relevant documentation in docs - should we document linters anywhere?
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG.md
  • Updated Gaia/Examples
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)
  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)

@codecov
Copy link

codecov bot commented Jul 8, 2018

Codecov Report

Merging #1593 into develop will decrease coverage by 0.19%.
The diff coverage is 59.77%.

@@            Coverage Diff             @@
##           develop    #1593     +/-   ##
==========================================
- Coverage    64.31%   64.12%   -0.2%     
==========================================
  Files          122      122             
  Lines         6704     6754     +50     
==========================================
+ Hits          4312     4331     +19     
- Misses        2147     2177     +30     
- Partials       245      246      +1

@ValarDragon
Copy link
Contributor Author

Fixing client/keys/add.go:49::warning: cyclomatic complexity 16 of function runAddCmd() is high (> 10) (gocyclo) will close #1446. Seeing as we already knew this is a problem, this makes me think that our cyclomatic complexity maximum should be less than 16. (So that this would fail linting)

Gocyclo is a code complexity linter. It uses cyclomatic complexity.
Cyclomatic complexity essentially measures the number of different
paths code could go through. (The conditional in a for loop counts
as adding one path) It looks at this on a per-function level. The
idea that this would be enforcing is that if there are too many
different paths code can go through in a function, it needs to be
better split up. (A function with too many code paths is hard to
reason about)

The complexity which we want the linter to start failing on is
configurable. The default is 10. Change the "Cyclo" parameter in
`tools/gometalinter.json` to try other values.
* Adds a Min function to Int, and uses that in the slash function
* Adds a getHeight helper function to iavlstore
* Adds a splitPath function to baseapp
* Changes cyclo param from 10 to 11
@ValarDragon
Copy link
Contributor Author

ValarDragon commented Jul 9, 2018

I added nolint in many places. For a couple of them the relevant code is going to be replaced in upcoming refactors / in handling of other issues.

Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

Tested ACK

@cwgoes cwgoes merged commit 777d7be into develop Jul 9, 2018
@cwgoes cwgoes deleted the dev/add_gocyclo branch July 9, 2018 23:45
@ebuchman
Copy link
Member

Awesome, thanks!

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