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

Add gometalinter support #92

Merged
merged 8 commits into from
Jun 20, 2017
Merged

Conversation

mjtrangoni
Copy link
Contributor

Hi @joehandzik @roclark,

I am creating this PR adding gometalinter support. There is a bunch of different Linters that you can list with gometalinter -h, and you can disable some that are maybe not relevant to you.

Travis is failing of course, but you can just analyze what you want to fix and what you would simply want to disable.

I personally think this tool is much more powerful, and offer editor integration too!

@joehandzik
Copy link
Contributor

@mjtrangoni Robet and I have discussed this a few times since you submitted this PR, and we're not sure that we want to jump to this level of complexity yet. Are there particular things that any of these other linters catch that you think we're missing out on?

@mjtrangoni mjtrangoni force-pushed the gometalinter branch 3 times, most recently from 64ecef2 to ecfc1fa Compare June 18, 2017 16:37
@mjtrangoni
Copy link
Contributor Author

@joehandzik I think the default checks make sense (link), and I took the time to fix the trivial ones for you (I can rebase them all in one commit if you like it that way).
Another thing was to make gometalinter running well with TravisCI, which was not really easy, but it works like a charm now! I just had to disable two checks that took too long at Travis.

The main two things that I leave to you is to fix or disable the following ones,

>> linting code
sources/procfs.go:289::warning: cyclomatic complexity 18 of function (*lustreProcfsSource).Update() is high (> 10) (gocyclo)
sources/procfs_test.go:44::warning: cyclomatic complexity 14 of function TestGetJobStats() is high (> 10) (gocyclo)
sources/procfs.go:709::warning: cyclomatic complexity 11 of function (*lustreProcfsSource).parseFile() is high (> 10) (gocyclo)
sources/procfs.go:280:30:warning: error return value not checked (l.generateOSTMetricTemplates()) (errcheck)
sources/procfs.go:281:30:warning: error return value not checked (l.generateMDTMetricTemplates()) (errcheck)
sources/procfs.go:282:30:warning: error return value not checked (l.generateMGSMetricTemplates()) (errcheck)
sources/procfs.go:283:30:warning: error return value not checked (l.generateMDSMetricTemplates()) (errcheck)
sources/procfs.go:284:33:warning: error return value not checked (l.generateClientMetricTemplates()) (errcheck)
sources/procfs.go:285:34:warning: error return value not checked (l.generateGenericMetricTemplates()) (errcheck)
sources/procsys.go:91:25:warning: error return value not checked (l.generateLNETTemplates()) (errcheck)
  • gocyclo checks the Cyclomatic_complexity, and I think that this should be improved. @knweiss, do you agree with that?

  • errcheck shouldn`t be really important but would be also a pity to disable it.

@joehandzik
Copy link
Contributor

I would guess that we would disable gocyclo for now, I don't think builds should fail because of the complexity numbers it is checking against. We can make a quick fix for all of those errcheck issues, so I'm fine with leaving that enabled (we really should check all errors).

Thank you for taking the time to make these changes! We'll figure out a merge plan today between myself and @roclark.

@mjtrangoni
Copy link
Contributor Author

@joehandzik Perfect! About gocyclo, there is an option that you can increase,

--cyclo-over=10       Report functions with cyclomatic complexity over N (using gocyclo).

@joehandzik
Copy link
Contributor

@mjtrangoni I believe I have fixed the errcheck issues with PR #99. Can you add the --cycle-over flag to your commit where you change the flags in the Makefile, and set it to 19 (basically, make sure we don't regress with regard to cyclomatic complexity for now)?

@mjtrangoni
Copy link
Contributor Author

@joehandzik TravisCI seems to be happy now, but local is not the same for me. Look,

$ make
>> formatting code
>> linting code
lustre_exporter.go:88:8:error: assignment count mismatch (1 vs 2) (gotype)
Makefile:37: recipe for target 'gometalinter' failed
make: *** [gometalinter] Error 1

@joehandzik
Copy link
Contributor

Hmm, I saw an issue like that, looks like you still have the error defined somewhere in either sources/source.go or lustre_exporter.go. Not sure why the rebase didn't completely take care of your local code...

@joehandzik
Copy link
Contributor

@mjtrangoni Your code looks fine in your branch...are you sure you aren't running against something old, or maybe against your fork's master instead of the branch you rebased?

@joehandzik
Copy link
Contributor

Gonna merge, if we actually have problems after this we'll resolve.

@joehandzik joehandzik merged commit 296cb5b into HewlettPackard:master Jun 20, 2017
@mjtrangoni
Copy link
Contributor Author

Ok, Thanks! I will be checking this later directly from master

@joehandzik
Copy link
Contributor

A build worked for me without issue just now from master, so I expect everything to work fine on your end too.

@mjtrangoni
Copy link
Contributor Author

@joehandzik Just for the records, and as I had to do with Travis, this is a gotype issue.
You have to do go test -i, and gotype works again.

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.

2 participants