Closed mjtrangoni closed 7 years ago
@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?
@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.
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.
@joehandzik Perfect! About gocyclo, there is an option that you can increase,
--cyclo-over=10 Report functions with cyclomatic complexity over N (using gocyclo).
@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)?
@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
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...
@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?
Gonna merge, if we actually have problems after this we'll resolve.
Ok, Thanks! I will be checking this later directly from master
A build worked for me without issue just now from master, so I expect everything to work fine on your end too.
@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.
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!