-
Notifications
You must be signed in to change notification settings - Fork 51
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
Conversation
@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? |
64ecef2
to
ecfc1fa
Compare
@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). The main two things that I leave to you is to fix or disable the following ones,
|
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,
|
@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,
|
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. |
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!