sasstools / sass-lint

Pure Node.js Sass linting
MIT License
1.77k stars 532 forks source link

[sass] False positive indentation error by using a mixin on the second level selector #1084

Open rastersysteme opened 7 years ago

rastersysteme commented 7 years ago

What version of Sass Lint are you using? 1.10.2

Please include any relevant parts of your configuration

rules:
  indentation: 2

What did you do? Please include the actual source code causing the issue.

.foo
  +clearfix
  .bar
    +clearfix
  .baz
    margin: 0

As you see I’m using the old Sass indented syntax here.

But it doesn’t matter if I write:

.bar
  +clearfix

or

.bar
  @include clearfix

Both give me an error.

What did you expect to happen? The linting should pass without an error.

What actually happened? Please include any error messages given to you by Sass Lint. The linter throws an indentation error.

5:3  error  Expected indentation of 6 spaces but found 2  indentation

image

If you're using a IDE plugin have you tried the CLI too? Yes, both the CLI and Sublime Text 3 throw an error.

nl0 commented 7 years ago

i'm getting similar errors

jakiestfu commented 7 years ago

Halp. This fails too:

@mixin ui-from ($bp)
  @media only screen and (min-width: $bp + 1px)
    @content

@mixin ui-to ($bp)
  @media only screen and (max-width: $bp)
    @content
henrahmagix commented 7 years ago

Seems like this has been fixed in https://github.com/tonyganch/gonzales-pe/commit/da531859a579ee7a56265824221c6ee7075599cb (fixes https://github.com/tonyganch/gonzales-pe/issues/193).

The gonzales-pe playground, running from the latest in the dev branch (https://github.com/tonyganch/gonzales-pe/commit/ab24a90faf070bc7fa0ad28b3cc63849c4d3f6bc), produces the correct tree:

@mixin foo
    @media screen
        @content

@mixin other
    color: red
Parse tree ``` -> stylesheet | -> mixin | | -> atkeyword | | | -> ident | | | "mixin" | | -> space | | " " | | -> ident | | "foo" | | -> space | | "\n" | | -> block | | | -> space | | | " " | | | -> atrule | | | | -> atkeyword | | | | | -> ident | | | | | "media" | | | | -> space | | | | " " | | | | -> ident | | | | "screen" | | | | -> space | | | | "\n" | | | | -> block | | | | | -> space | | | | | " " | | | | | -> atrule | | | | | | -> atkeyword | | | | | | | -> ident | | | | | | | "content" | | | | | | -> space | | | | | | "\n" | | | -> declarationDelimiter | | | "\n" | -> mixin | | -> atkeyword | | | -> ident | | | "mixin" | | -> space | | " " | | -> ident | | "other" | | -> space | | "\n" | | -> block | | | -> space | | | " " | | | -> declaration | | | | -> property | | | | | -> ident | | | | | "color" | | | | -> propertyDelimiter | | | | ":" | | | | -> space | | | | " " | | | | -> value | | | | | -> ident | | | | | "red" | | | -> declarationDelimiter | | | "\n" ```

whereas v3.4.7 (latest release) produces this error.

@rastersysteme Your example also seems to have been fixed in the dev branch. I've successfully used sass-lint@1.10.2 with it and the error for the example above is gone.

I hope a new release of gonzales-pe is done soon! I've requested a new release =)

henrahmagix commented 7 years ago

@rastersysteme @nl0 @jakiestfu I've forked gulp-sass-lint, sass-lint, and gonzales-pe to use the latest AST to fix this. I was unable to accomplish this via npm-shrinkwrap.json in npm 4+ :(

npm install --save-dev git://github.com/henrahmagix/gulp-sass-lint.git#85608dacf2c259eceadf15102a6fb0377937f271
DanPurdy commented 7 years ago

The v4 updated will be landing is Sass-lint soon. Sorry for the delays here but life has got in the way for many of us.

This was indeed an issue with Gonzales where it incorrectly nested at-rules and mixins etc in the Sass format.

henrahmagix commented 7 years ago

Thanks!

rastersysteme commented 7 years ago

@DanPurdy It seems with the update of gonzales-pe parser to 4.1.1 in 1.11 that this bug is fixed now. Can you confirm this?

DanPurdy commented 7 years ago

@rastersysteme So the indentation error here seems to be working. Nevertheless I think there's a big in the empty-line-between-blocks rule that will affect this if you don't have spaces in between your class declarations. I'll look into this further hopefully tomorrow. The fix that rectifies the indentation though is already released for you to try.