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 stylelint checker #903

Merged
merged 1 commit into from Mar 23, 2017

Conversation

@PhilippBaschke
Contributor

PhilippBaschke commented Mar 6, 2016

These changes should enable the css-stylelint checker in css-mode and scss-mode.

It covers 3 different scenarios:

1) Normal errors and warnings

# .stylelintrc.yaml
rules:
  string-quotes: double
  indentation: [2, { "warn": true }]
/* style.css */
h1 {
  color: 'red'; /* error: single quote */
    margin: 0 auto; /* warning: wrong intentation */
}

stylelint output (missing error level information):

$ stylelint style.css
2:10    Expected double quotes (string-quotes)
3:5 Expected indentation of 2 spaces (indentation)

That's why I used the JSON formatter and wrote my own :error-parser

$ stylelint --formatter json style.css
[{"source":"style.css","deprecations":[],"errored":true,"warnings":[{"line":2,"column":10,"rule":"string-quotes","severity":"error","text":"Expected double quotes (string-quotes)"},{"line":3,"column":5,"rule":"indentation","severity":"warning","text":"Expected indentation of 2 spaces (indentation)"}]}]

2) Syntax errors

/* style.css */
h1 {
  color: 'red; /* syntax error: Unclosed quote */
    margin: 0 auto;
}
$ stylelint --formatter json style.css
CssSyntaxError: style.css:2:10: Unclosed quote

3) Configuration errors

# .stylelintrc.yaml
# should be rules instead of rles
rles:
  string-quotes: double
  indentation: [2, { "warn": true }]
$ stylelint --formatter json style.css
Error: No rules found within configuration. Have you provided a "rules" property?

The :error-filter function adds a line (line number 1) to these errors to be able to show them.

Questions

  • What is the best way to define this checker for css-mode and scss-mode?
  • What is the best way to use a combination of an :error-parser and some form of :error-patterns?
  • Is it necessary to show the Syntax errors and Configuration Errors or should flycheck just fail if these happen?
  • Should I rather try to create a PR at stylelint that changes their text formatter to output the error level and then create a simple checker that just uses :error-patterns for all 3 scenarios?

Edit: Connects to #785

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Mar 7, 2016

Thank you very much for this pull request. I'd really like to merge it, but I'm afraid that there's some way to go on stylelint's side before we can consider it.

As far as I can see from your examples stylelint's output is a broken mess. Not using JSON in some cases leads the whole point and purpose of choosing JSON in the first place—easy and safe interoperability—ad absurdum. As far as I'm concerned, that's a show-stopper for integration in Flycheck.

Stylelint needs to provide a clean output before we can go on, either JSON for everything (at the very least for syntax errors) or a plain text format that includes everything that's required for Flycheck. Either fix needs changes to stylelint, and there's no point I think in discussing your other questions before.

Would you volunteer to open a corresponding pull request for styllelint?

@lunaryorn lunaryorn changed the title from 785 stylelint checker to Add stylelint checker Mar 7, 2016

@PhilippBaschke

This comment has been minimized.

Contributor

PhilippBaschke commented Mar 8, 2016

Scenario 2) and 3) are Errors that are thrown. They also include a trace that I omitted. So maybe that's why they are not formatted with JSON if you choose --formatter json.
I found a promising Issue that is already WIP at stylelint: stylelint/stylelint#843
I'll wait how that develops and do my own pull request otherwise.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Mar 8, 2016

@PhilippBaschke Would you mind to open an issue wrt syntax errors? Configuration errors aside, at least syntax errors should be reported properly and not thrown as exceptions, imho. I think it'd be good to make the stylelint developers aware that this is an issue for integration with 3rd party tools, isn't it?

@PhilippBaschke

This comment has been minimized.

Contributor

PhilippBaschke commented Mar 8, 2016

Ok. I'll do that.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Mar 8, 2016

@PhilippBaschke Thank you very much 👍 😊

@PhilippBaschke

This comment has been minimized.

Contributor

PhilippBaschke commented Mar 8, 2016

@lunaryorn Sorry for dragging you into the discussion but would you mind helping to clarify why it would be a good idea to not throw syntax errors. I feel like you have way more knowledge on the topic than I do. 😊 (the stylelint maintainers suggest handling it in flycheck). Thank you!

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Mar 8, 2016

I'm not sure whether I understand you. How are we supposed to handle syntax errors in Flycheck? Flycheck doesn't parse CSS nor does it check CSS syntax.

All we can do is to rely upon the linter to tell us about what it found, and listening to the linter is a lot easier if it speaks a single format for all errors it reports.

In other words, it's the difference between parsing two formats (the JSON for warnings and the plain text for syntax errors) and parsing a single format (just JSON). You've seen yourself that the former is a lot harder, and as far as I'm concerned that's too much and too brittle code for us to maintain in Flycheck.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Apr 14, 2016

With stylelint/stylelint#883 being implemented we only need to wait for the next stylelint release to merge this syntax checker.

@PhilippBaschke Meanwhile, would you mind to update this pull request to add the new syntax checker to doc/languages.rst so that it's included in our manual?

@PhilippBaschke

This comment has been minimized.

Contributor

PhilippBaschke commented Apr 14, 2016

@lunaryorn
I'll look into it tomorrow. I would do the following things:

  • Rebase
  • Replace JSON parser with Patterns
  • Document the Checker for CSS
  • --quiet flag
  • Switch back to JSON parser
  • Make the Checker available for SCSS
  • Make the Checker available for LESS?!

Does that sound like a good plan? I'm never sure if rebasing is OK or not.

stylelint does support scss, less (experimental) and sugarcss syntax with the --syntax flag. Is there a best practice to make it available for different modes with as little duplication as possible? Is it even desirable? I guess most people write CSS in another mode these days?!

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Apr 14, 2016

@PhilippBaschke Sounds great! Rebasing certainly makes things easier for us as we cherry pick your changes onto master, but it is by no means mandatory.

To switch syntax dynamically you can use a custom (eval) argument that dispatches based on the current major mode. Take a look at the c/c++-clang syntax checker, which does a similar thing to dispatch between C and C++. I hope that this helps; please don't hesitate to ask if there's still anything unclear.

@PhilippBaschke

This comment has been minimized.

Contributor

PhilippBaschke commented Apr 14, 2016

Cool. Thanks for the tip, will look into c/c++-clang for inspiration. How should I name a checker that supports multiple "languages" since it normally is language-linter or is css-stylelint OK?

@Simplify

This comment has been minimized.

Member

Simplify commented Apr 15, 2016

If c/c++ checkers have c/c++-(linter) name, my guess is that flycheck-css/scss/less-stylelint is the best choice. I'll skip SugarSS, there is no GNU Emacs mode for that.

Please, also implement --quiet flag. That flag is used to display only errors and skip warnings. Most of the warnings can automatically be fixed using cssfmt.el. cssfmt.el uses stylefmt npm package that uses same config as stylelint.

I was planning to create separate flycheck extension for this, but now I see that work is already started. If you need any help, let me know. Just update your branch on GitHub with latest flycheck code.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Apr 15, 2016

@PhilippBaschke I think we can also easily duplicate the checker definition for css-stylelint, scss-stylelint, etc… the logic is in the JSON parsing and the error filter, and these can easily be shared among multiple checkers.

I think I'd prefer this approach, because it let's us configure :next-checkers individually per language.

@PhilippBaschke PhilippBaschke force-pushed the PhilippBaschke:785-stylelint-checker-draft branch from 8943a03 to a17d5f6 Apr 18, 2016

@PhilippBaschke

This comment has been minimized.

Contributor

PhilippBaschke commented Apr 19, 2016

I am not really sure why flycheck-add-overlay/right-position-in-narrowed-buffer failed. When I run make unit on my system it passes. It seems like it timed out. Do you have any idea? 😕

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Apr 19, 2016

I wouldn't worry too much about that error; looks like it was mostly a fluke :)

@PhilippBaschke

This comment has been minimized.

Contributor

PhilippBaschke commented Apr 19, 2016

Is duplicating checker definitions to support multiple modes still the way to go after switching to :error-patterns?

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Apr 19, 2016

I'll defer to @lunaryorn for final confirmation, but I'd say yes; it's easy to share the error patterns between checkers, right?

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Apr 19, 2016

@cpitclaudel We can't share :error-patterns between checkers defined with flycheck-define-checker (though it's possible with the underlying flycheck-define-command-checker, but I wouldn't go that route).

But, @PhilippBaschke, why did we switch to :error-patterns? I thought we'd stick with JSON parsing now that stylelint has unified reporting for all kinds of errors?

@PhilippBaschke

This comment has been minimized.

Contributor

PhilippBaschke commented Apr 19, 2016

I thought it makes more sense to use flychecks functionality instead of adding new functionality (the JSON parse method). It also makes it possible to catch errors that fall into scenario 3 (e.g. no configuration, error in configuration - because they are still thrown).

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Apr 19, 2016

@PhilippBaschke Ah, well, now I see where you're coming from. Your position is reasonable, but we prefer to use structured output formats in Flycheck if possible. In my experience these are simply less fragile and come with less maintenance cost on the long run. They are also more open to reuse, just check out how many checkers use the built-in checkstyle error parser.

As for configuration errors, you can handle these in a custom error parser as well. Take a look at flycheck-parse-scss-lint where we wrap a custom error parser around the standard flycheck-parse-checkstyle to catch a peculiar error of scss-lint.

I'd be very grateful if you could go for the JSON parsing route again if you don't mind. I know I'm asking a lot since it comes down to refactoring a lot of your changes again, for seemingly no gain, 😊 but you'd really make maintenance for us a lot easier. ☺️

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Apr 19, 2016

@PhilippBaschke A further point pro error parses is that at least within our current infrastructure these are much easier to test independently.

@PhilippBaschke

This comment has been minimized.

Contributor

PhilippBaschke commented Apr 19, 2016

OK, I'll switch back 😁 I learned a lot about rx at least 😆

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Apr 19, 2016

@PhilippBaschke Thank you very much ☺️

@CLAassistant

This comment has been minimized.

CLAassistant commented Mar 13, 2017

CLA assistant check
All committers have signed the CLA.

@PhilippBaschke PhilippBaschke force-pushed the PhilippBaschke:785-stylelint-checker-draft branch 3 times, most recently from 99d58af to 27cb488 Mar 13, 2017

@PhilippBaschke

This comment has been minimized.

Contributor

PhilippBaschke commented Mar 13, 2017

I haven't checked if everything still works (and will go to bed now) but everything else should be ready for review. Will do smoke tests tomorrow after work ☺️ Thanks for being so patient! 😌

@PhilippBaschke

This comment has been minimized.

Contributor

PhilippBaschke commented Mar 14, 2017

Manual tests look good. Tested the following for css, scss and less:

  • Errors
  • Warnings
  • Syntax errors
  • Configuration errors (recoverable by stylelint and errors in the file structure)
  • Missing configuration
  • Deprecations in the configuration

So from my perspective this is ready to go (unless you have review remarks which I am happy to fix). 🎉

I am also happy to support any upcoming problems with this checker 😌

@Simplify

This comment has been minimized.

Member

Simplify commented Mar 21, 2017

Hi @PhilippBaschke sorry for late reaction. I tested this, it works :)

Can you also update CHANGES.rst file and add that update to your commit?

@cpitclaudel Can you take last look? Many of will like to get this in Flycheck, stylelint is currently best tool for linting css/scss files.

flycheck.el Outdated
about the JSON format of stylelint.
See URL `http://edward.oconnor.cx/2006/03/json.el' for information
about json.el."

This comment has been minimized.

@cpitclaudel

cpitclaudel Mar 21, 2017

Member

json.el is part of Emacs now; no need to add a link here.

flycheck.el Outdated
(flycheck-parse-stylelint-json output checker buffer)
;; The output could not be parsed as JSON
(json-readtable-error

This comment has been minimized.

@cpitclaudel

cpitclaudel Mar 21, 2017

Member

Would it be better to catch any json-error here?

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Mar 21, 2017

LGTM. Just two small comments.

Add stylelint checker for CSS, SCSS and Less
Define a checker for stylelint that parses JSON

The stylelint CLI can format its output as text or JSON. The text
formatter is not printing a severity keyword. That's why I used the JSON
formatter and added a parser for the JSON output.

See: https://stylelint.io/

Show CssSyntaxErrors with checker css-stylelint

When the CSS contains syntax errors and it is passed to the
css-stylelint checker, the error output does not contain JSON data and
therefore leads to a "checker probably flawed" error message. To avoid
this error message and to show the user where the CSS syntax error is,
the stylelint CssSyntaxError message is parsed and shown in the right
position.

I used the build in flycheck-parse-with-patterns and an error pattern in
:error-patterns to introduce as little new functionality as possible.

Show stylelint configuration errors in line 1

A problem with the stylelint configuration is not returned in JSON
format and has to be parsed with an error pattern aswell. The error
pattern does not contain a line number. Because errors without line
numbers can't be shown, an error-filter is needed to add a line number
of 1. This filter also prepends the error message with "Configuration
error: " to tell the user that the error occured because of a
misconfiguration of stylelint.

The flycheck-sanitize-errors filter is needed because the error-pattern
for CssSyntaxErrors returns messages with unnecessary whitespace.

Add config-file-var for stylelint checker

The stylelint library has a good lookup mechanism for the default
configuration, thats why the standard value of the config-file-var
flycheck-stylelintrc is set to nil.
See: http://stylelint.io/user-guide/configuration/#loading-configuration

Enable css-stylelint in scss-mode

stylelint can parse scss syntax (with postcss-scss) and can be used in
scss-mode aswell. To avoid having to specify an extra checker (and
duplicate some code), I decided to just activate the "--syntax" option
of stylelint. It should work with normal css files aswell.

Replace stylelint JSON parser with :error-patterns

The CLI output of stylelint was improved and shows error severity now.
To avoid adding custom code to flycheck the JSON parser can be replaced
with error-patterns. This is possible because of these two PRs:

- stylelint/stylelint#1023
  Show CssSyntaxErrors along the normal linter warnings (threw an
  Exception before)

- stylelint/stylelint#1056
  Show an icon for severity and show rule name in extra column

The :error-patterns cover the following scenarios:

- Linting errors of any severity
  The severity is shown with an icon from the npm package log-symbols.
  These vary depending on the operating system (different icons for
  windows). Thats why the error and warning pattern contain two
  different symbols.
  See: https://github.com/sindresorhus/log-symbols

  It is possible to use custom severities in stylelint. The info pattern
  matches all linting errors with a severity other than error or
  warning. Info seems to fit best because the user could use error or
  warning otherwise.

- Deprecation Warnings and Invalid Options
  The output also informs the user about deprecated rules and invalid
  configuration options. This helpful information is shown in the first
  line.

- Errors
  Some things result in a thrown Error. For example, a missing
  configuration file, errors in the YAML or JSON file, no "rules"
  property in the configuration, using non-existent rules. Especially
  missing configurations files or errors in the configuration file seem
  likely. To avoid a flycheck error ("checker definition probably
  flawed") in these cases, their error message is shown in the first
  line.

Add --quiet flag to css-stylelint syntax checker

Most warnings can be automatically fixed and some user might not want
to show them (the quiet flag only shows errors with severity "error").

Replace :error-patterns with :error-parser

All flycheck checkers should prefer structured output. That's why the
stylelint JSON formatter with a custom :error-parser is the recommended
way.

The JSON parser gets deprecation warnings, invalid option errors and
linting errors from stylelint and turns them into flycheck errors.

stylelint sometimes throws errors and does not output JSON. The error
parser handles this case and uses a regular expression to get an error
message.

Move stylelint for scss-mode in extra checker
Add a stylelint checker for less-css-mode

Save common args in a constant and use the same config and options
variable in all checkers to reduce duplication.

@PhilippBaschke PhilippBaschke force-pushed the PhilippBaschke:785-stylelint-checker-draft branch from 27cb488 to 927ede5 Mar 22, 2017

@PhilippBaschke

This comment has been minimized.

Contributor

PhilippBaschke commented Mar 22, 2017

Fixed the remarks and added a line to the CHANGES.rst. Should be read to merge. 🎉

@nickmccurdy

This comment has been minimized.

nickmccurdy commented Mar 22, 2017

I think the CI build is stalling: https://travis-ci.org/flycheck/flycheck/jobs/214006637

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Mar 23, 2017

@nickmccurdy Thanks, I restarted it.

@Simplify

This comment has been minimized.

Member

Simplify commented Mar 23, 2017

@PhilippBaschke Thanks a lot, this can be merged now :)

@cpitclaudel cpitclaudel merged commit 83a9a7b into flycheck:master Mar 23, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Mar 23, 2017

Yay! :) Good job everyone. Thanks so much for your patience @PhilippBaschke

@fmdkdd fmdkdd moved this from TODO to Done in New syntax checker chaining Mar 23, 2017

@PhilippBaschke

This comment has been minimized.

Contributor

PhilippBaschke commented Mar 23, 2017

Thanks for merging! 🎉 😌

@nickmccurdy

This comment has been minimized.

nickmccurdy commented Mar 30, 2017

Nice job! Would it be possible to add package.json configuration support (which is officially supposed by stylelint)? The checker is enabling just fine but it isn't noticing my configuration because I don't use separate config files.

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Mar 30, 2017

@nickmccurdy Sure, feel free to open a PR or issue. Look at flycheck-def-config-file-var and how it's done in the tslint checker for example.

@PhilippBaschke

This comment has been minimized.

Contributor

PhilippBaschke commented Mar 30, 2017

@nickmccurdy Should I do it or are you going to do it? ☺️

@nickmccurdy nickmccurdy referenced this pull request Mar 31, 2017

Closed

Add Stylelint checker #785

@nickmccurdy

This comment has been minimized.

nickmccurdy commented Mar 31, 2017

Nevermind, I was wrong. The stylelint checker actually does support package.json configuration. You just need to leave flycheck-stylelintrc at its default value, nil.

PhilippBaschke added a commit to PhilippBaschke/stylelint that referenced this pull request Apr 3, 2017

Add Flycheck to the list of editor plugins
Flycheck is a popular syntax checking extension for Emacs. It recently added
support for stylelint.

See: flycheck/flycheck#903

jeddy3 added a commit to stylelint/stylelint that referenced this pull request Apr 3, 2017

Add Flycheck to the list of editor plugins (#2468)
Flycheck is a popular syntax checking extension for Emacs. It recently added
support for stylelint.

See: flycheck/flycheck#903

@PhilippBaschke PhilippBaschke deleted the PhilippBaschke:785-stylelint-checker-draft branch Apr 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment