sds / scss-lint

Configurable tool for writing clean, consistent SCSS
MIT License
3.65k stars 466 forks source link

Add compatibility for .sass files #38

Open dsandstrom opened 11 years ago

dsandstrom commented 11 years ago

Is sass compatibility planned? If not, are you guys open to pull requests? Thanks.

sds commented 11 years ago

Thanks for your interest in scss-lint, @dsandstrom.

We currently do not have any plans to support Sass syntax, as internally we only use SCSS syntax. Furthermore, SCSS is considered the "main" syntax (https://github.com/nex3/sass/blame/aec3cc45e2e3e87afebede8da7eb012179a38293/README.md#L8)

Having said that, there is no strong reason we could not support both, but since there are some SCSS-specific linters we would have to start marking linters as belonging to one syntax form or the other.

We are certainly open to pull requests if you are interested in sending them our way.

kangax commented 10 years ago

I would love to see this as well, as there seems to be no SASS linter out there at all at the moment...

perry commented 10 years ago

+1

henrahmagix commented 10 years ago

+1

rogerhutchings commented 10 years ago

This would be awesome, +1

grahamgilchrist commented 10 years ago

I'd really like to see support for this as well. Although SCSS is now the 'main syntax' it wasn't the case prior to Sass 3, so there are a lot of legacy codebases out there using the Sass syntax, as well as developers who prefer the cleaner indented style which is similar to python or ruby code style.

It's great that you are open to pull-requests, but as you say, the linters would likely need to be rebuilt for the different style.

mprins commented 10 years ago

legacy code can easily be converted/modernized to the preferred syntax using sass-convert style.sass style.scss (though for "fixing" developer preference something else would propbably be required :innocent: )

This conversion will yield the benefit of being able to use tools like SassDoc/sassdoc

see: http://sass-lang.com/documentation/file.SASS_REFERENCE.html#syntax

alp82 commented 9 years ago

+1

kangax commented 9 years ago

Chiming in 9 months later — is there any hope for this at all?

sds commented 9 years ago

There's always hope.

Technically speaking, this should not be terribly difficult to do. However, this will be a very tedious change, as many linters will need to have their tests written to check both Sass and SCSS syntaxes to ensure they still work as expected with Sass-style code.

I'm happy to review a pull request if someone is willing to tackle this task. At this point in time, I can't offer to take it on myself, as it is indeed a significant undertaking (we have over 1,000 examples in our test suite, though the actual number of Sass-style code samples you would need to write would probably be about half that).

lencioni commented 9 years ago

Would it be helpful if we started a branch where the scss-lint community could collaborate on landing this feature? Maybe if everyone pitches in, the tediousness of the task would be minimized.

sds commented 9 years ago

I'm open to any suggestions, but someone is going to have to take the torch and get the ball rolling.

There's also a path where we develop on master, since much of the work can be done without "releasing" it. I believe a potential path is to update the before(:each) block in spec/spec_helper.rb to look for scss and sass variables and parse each separately, run the linter being tested against each, and ensure their output matches.

This saves us from having to rewrite tests, and will for the most part just need to change tests from:

context 'when rule set contains no duplicates' do
  let(:css) { <<-CSS }
    p {
      margin: 0;
      padding: 0;
    } 
  CSS

  it { should_not report_lint }
end

...to:

context 'when rule set contains no duplicates' do
  let(:sass) { <<-CSS }
    p
      margin: 0
      padding: 0
  CSS

  let(:scss) { <<-CSS }
    p {
      margin: 0;
      padding: 0;
    } 
  CSS

  it { should_not report_lint }
end
camerond commented 9 years ago

I'd also love to see this. Was looking into adding scss-lint to our projects but our team uses Sass syntax exclusively.

mdiebolt commented 9 years ago

@sds I started pursuing the approach you outlined for adding .sass support. You can see my changes at: https://github.com/mdiebolt/scss-lint/commit/0710e4440bb059bbbb428ffba3e1d34b47e4f565

I ran into trouble and am pretty stumped.

Running

bundle exec rspec spec/scss_lint/linter/bang_format_spec.rb:16

I get the following error: http://monosnap.com/image/fd2Zwt4FXVe7dbW8MWLWuUvxusYsx0

The problem starts when generating the AST at: https://github.com/mdiebolt/scss-lint/blob/master/lib/scss_lint/engine.rb#L27

Digging in, the code errors at: https://github.com/sass/sass/blob/stable/lib/sass/tree/node.rb#L193

Working through the stack trace I found the offending node:

#<Sass::Tree::PropNode>
  self.name  # => ["color"]
  self.value # => [#000000] 
  self.children # => [#000000]

#000000 doesn’t have the expected each method, and the code blows up.

The strange thing is that if I spin up irb and run the same series of commands (I checked to make sure rspec and irb sessions were running the same version of Ruby and using the same version of the sass gem), it generates the tree correctly.

require "sass"

# using the same parameters passed in at: https://github.com/mdiebolt/scss-lint/blob/master/lib/scss_lint/engine.rb#L51
engine = Sass::Engine.new("p\n  color: #000\n", cache: false, syntax: :sass) 

engine.to_tree # => correct SASS AST

Inspecting the corresponding node in my irb example I found:

#<Sass::Tree::PropNode>
  self.name  # => ["color"]
  self.value # => [#000000] 
  self.children # => []

I’m not sure why self.children is being set differently in those two examples. The only thing I can think is that somewhere scss-lint sets some configuration options on Sass or Sass::Engine. Any ideas?

sds commented 9 years ago

Hey @mdiebolt, thanks for digging in to adding this feature.

The issue you're seeing is due to the monkey patching we do to the Sass parse tree nodes in https://github.com/causes/scss-lint/tree/master/lib/scss_lint/sass.

The monkey patching is necessary so the linters can easily define visit_script_* methods to visit Sass::Script::Tree::Node parse nodes while scanning for lints.

I don't have the time to dig into this deeper, but it's odd that this is an issue when parsing .sass files but not .scss files. If you can figure out why that is, a workaround may present itself.

tbremer commented 9 years ago

Jumping in way after a lot of people have made suggestions (and I didn't read them all).

Is it possible to run a sass-convert and then lint the converted output?

sds commented 9 years ago

I'm not sure how that addresses the problem, @tbremer. All that would give you is a converted document, with lints which may not apply to the original document you had (there are some lints that are necessarily SCSS-specific or vice versa).

tbremer commented 9 years ago

Ahh, good point.

mdiebolt commented 9 years ago

@sds Thanks for the quick reply!

I've made good progress thanks to your help and should have a passing .sass test suite by the end of the week.

At that point, I'll need to figure out a way to DRY up the specs and support .sass in the CLI.

CREEATION commented 9 years ago

I'm so waiting for this /o/ thanks for your efforts!

batizhevsky commented 9 years ago

@CREEATION Looks line https://github.com/mdiebolt/scss-lint work for me. Thank you @mdiebolt

sds commented 9 years ago

@mdiebolt Anything holding you back from submitting your changes back into the upstream project? (I have not tested your implementation, so I don't know if it's comprehensive or not)

brunowego commented 9 years ago

+1

CREEATION commented 9 years ago

yep, :+1:! @mdiebolt

bartveneman commented 9 years ago

For what it's worth, using the scss-lint plugin in sublime text already gives some feedback on .sass files. Of course not all of it, but for me it's enough to work with for the time being.

Plummat commented 9 years ago

So... has this been resolved?

ryanmclaughlin commented 9 years ago

+1 Wondering this as well for Atom

sds commented 9 years ago

We haven't heard back from @mdiebolt about merging his work into the main project. If you'd like to see this, you can nudge him for us. :wink:

mdiebolt commented 9 years ago

I ran into lots of problems with the monkey patched Sass::Tree which caused line numbers to be incorrectly reported to the linter.

Also, the way I have the specs set up is not DRY at all. I split out specs into an "scss" context and a "sass" context, which makes for more than twice as much spec code: https://github.com/mdiebolt/scss-lint/commit/5027102cac9464755ed18e385f5e3ba81785674d

A good amount of the linters work on my fork and there are some which are moot with Sass because they are language compile errors.

If someone wants to poke around my code and try to figure out how to work around the patched Sass internals that would be helpful. I'm not going to spend more time working on it.

skrzyszewski commented 8 years ago

+1

blackst0ne commented 8 years ago

Any progress here? It would be very nice to have support for sass files.

batizhevsky commented 8 years ago

@blackst0ne you could check http://stylelint.io/ they support sass еркщгпре postcss plugin