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 go-megacheck #1290

Merged
merged 1 commit into from Jul 22, 2017
Merged

Add go-megacheck #1290

merged 1 commit into from Jul 22, 2017

Conversation

@mpolden
Copy link
Contributor

@mpolden mpolden commented Jul 22, 2017

I was initially going to add a checker for staticcheck, but then I found megacheck, which runs both the gosimple, staticcheck and unused tools.

This replaces the existing gosimple checker, as it's no longer needed.

Copy link
Member

@fmdkdd fmdkdd left a comment

Thanks. go seems to have a lot of external tools. Good for them!

You'll want to change the line in CHANGES.rst as well.

flycheck.el Outdated
`staticcheck' and `unused'. When nil, all checkers will be
enabled. "
:type '(choice (repeat :tag "Disabled checkers"
(string :tag "Checker name")))

This comment has been minimized.

@fmdkdd

fmdkdd Jul 22, 2017
Member

I think you can use const instead of string here to not allow arbitrary strings, but only the names of the checkers that make sense.

@mpolden
Copy link
Contributor Author

@mpolden mpolden commented Jul 22, 2017

go seems to have a lot of external tools. Good for them!

Yes, Go and the community seem to follow the UNIX philosophy closely, e.g "Do one thing and it well".

You'll want to change the line in CHANGES.rst as well.

Done.

I think you can use const instead of string here to not allow arbitrary strings, but only the names of the checkers that make sense.

Done, not that familiar with customize as I rarely use it. Changed to set and const. Verified expected behavior by customizing the variable.

PTAL. Love the rigorous testing and reviewing in this project by the way! 👍

@fmdkdd
fmdkdd approved these changes Jul 22, 2017
@fmdkdd
Copy link
Member

@fmdkdd fmdkdd commented Jul 22, 2017

LGTM.

Love the rigorous testing and reviewing in this project by the way!

Thanks! I guess that's a consequence of having most users pulling from master via MELPA to get Flycheck. You really don't want to merge stuff without taking a good look at it ;)

@cpitclaudel Want to double check?

@cpitclaudel
Copy link
Member

@cpitclaudel cpitclaudel commented Jul 22, 2017

LGTM. Just curious though — shouldn't we have both simplecheck and megacheck? Or do we expect everyone to just install megacheck?

@dominikh
Copy link
Member

@dominikh dominikh commented Jul 22, 2017

Using megacheck will be a lot more efficient than running gosimple, staticcheck and unused separately, so it could be worth only supporting megacheck and pushing users in that direction.

@cpitclaudel
Copy link
Member

@cpitclaudel cpitclaudel commented Jul 22, 2017

Ok, works for me :)

@cpitclaudel cpitclaudel merged commit 0ae3d41 into flycheck:master Jul 22, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@cpitclaudel
Copy link
Member

@cpitclaudel cpitclaudel commented Jul 22, 2017

Thanks @mpolden! Congrats on this nice contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants