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 slim-lint checker definition #1013

Merged
merged 4 commits into from Jul 18, 2016

Conversation

@ibrahima
Copy link
Contributor

commented Jul 13, 2016

Slim-lint is a third-party linter for slim templates.

See https://github.com/sds/slim-lint

I hope this was formatted correctly, first time contributing to flycheck! Currently there's no option to read from stdin so I couldn't set that, which is kind of annoying. Also, not sure if I set :next-checkers correctly or if the order matters. I think slim-lint also includes the basic slim errors so not sure if having both enabled causes problems, or what happens if the executable is not found (I assume no errors are thrown if it's not installed, right?)

Thanks!

flycheck.el Outdated

See URL `https://github.com/sds/slim-lint'."
:command ("slim-lint" "--reporter=checkstyle"
source-original)

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Jul 13, 2016

Member

Must this be source-original? Wouldn't source (or even source-inplace) work?

This comment has been minimized.

Copy link
@ibrahima

ibrahima Jul 13, 2016

Author Contributor

I have no idea, I just found an example for defining custom checkers online and modified it. Probably should have gone to the source rather than random online examples though ;). It seems like source works, so that the display isn't lagging behind until you save. Will change!

flycheck.el Outdated
source-original)
:error-parser flycheck-parse-checkstyle
:modes slim-mode
:next-checkers ((info . slim)))

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Jul 13, 2016

Member

@lunaryorn will confirm this, but I would expect slim-lint to be the next-checker of slim, not the other way around.

This comment has been minimized.

Copy link
@ibrahima

ibrahima Jul 13, 2016

Author Contributor

That would seem correct to me as well but I wasn't sure how the precedence works and how it works if you don't have one or the other installed.

@cpitclaudel

This comment has been minimized.

Copy link
Member

commented Jul 13, 2016

Great, thanks! Please check my inline comments.

@ibrahima

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2016

Thanks! Addressed your comments and force pushed. Any idea what the CI failure is about? I added a doc string correctly I think (again, just following what I see elsewhere in the file) so I don't understand what it's saying (seems like it expects slim-lint to not have a doc string?).

@ibrahima ibrahima force-pushed the ibrahima:add-slim-lint branch from 4b85c07 to ae7aca7 Jul 13, 2016

Add slim-lint checker definition
Slim-lint is a third-party linter for slim templates.

See https://github.com/sds/slim-lint

@ibrahima ibrahima force-pushed the ibrahima:add-slim-lint branch from ae7aca7 to 3105ffc Jul 13, 2016

@Simplify

This comment has been minimized.

Copy link
Member

commented Jul 13, 2016

Hi @ibrahima,

CI failure comes because documentation for this checker is missing in doc/languages.rst file.
Can you fix that? There is already slim section in that file, just add extra checker and short description with URL that points to slim-lint page.

@ibrahima

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2016

Oh cool, done!

@@ -972,6 +972,10 @@ to view the docstring of the syntax checker. Likewise, you may use

Check Slim using the `Slim <http://slim-lang.com/>`_ compiler.

.. syntax-checker:: slim-lint

Check Slim best practices using the `slim-lint <https://github.com/sds/slim-lint>`_ linter.

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Jul 14, 2016

Contributor

Please wrap this line with Alt+Q (i.e. fill-paragraph), as it's beyond our 80 character limit.

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Jul 14, 2016

@ibrahima Please add this change to CHANGES.rst (take a look at release 27 to see how we document syntax checkers in the changelog).

If you'd like please also feel free to add yourself to the Contributors section in doc/community/people.rst 😊

Add changelog entry, add self to contributors
Also wrapped a long line in documentation.
@ibrahima

This comment has been minimized.

Copy link
Contributor Author

commented Jul 14, 2016

Done!

CHANGES.rst Outdated
@@ -8,6 +8,10 @@
- ``flycheck-go-build-install-deps`` turns on dependency installation for ``go test``
as well as ``go build`` [GH-1003]

- New syntax checkers:

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Jul 15, 2016

Contributor

Ah, thanks, but could you just move this item above - New features? Generally we list breaking changes, then new syntax checkers, and then other new features, because I think that support for new languages is more important to most users than other options.

@ibrahima Could you just make this final change?

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2016

@ibrahima Finally, please sign our CLA to state your explicit consent with Flycheck's licenses for your contribution 😊

@ibrahima

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2016

Done!

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2016

LGTM

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2016

@cpitclaudel Would you take a look?

@cpitclaudel

This comment has been minimized.

Copy link
Member

commented Jul 17, 2016

LGTM :)

@lunaryorn lunaryorn merged commit cc95b39 into flycheck:master Jul 18, 2016

3 checks passed

approvals/lgtm this commit looks good
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

@ibrahima ibrahima deleted the ibrahima:add-slim-lint branch Jul 25, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.