scss-lint icon indicating copy to clipboard operation
scss-lint copied to clipboard

Add compatibility for .sass files

Open dsandstrom opened this issue 12 years ago • 32 comments

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

dsandstrom avatar Sep 04 '13 21:09 dsandstrom

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.

sds avatar Sep 05 '13 04:09 sds

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

kangax avatar Mar 24 '14 21:03 kangax

+1

perry avatar Aug 29 '14 09:08 perry

+1

henrahmagix avatar Aug 29 '14 09:08 henrahmagix

This would be awesome, +1

rogerhutchings avatar Aug 29 '14 09:08 rogerhutchings

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.

grahamgilchrist avatar Aug 29 '14 09:08 grahamgilchrist

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

mprins avatar Aug 29 '14 10:08 mprins

+1

alp82 avatar Dec 26 '14 17:12 alp82

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

kangax avatar Dec 26 '14 18:12 kangax

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).

sds avatar Dec 29 '14 07:12 sds

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.

lencioni avatar Dec 29 '14 21:12 lencioni

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

sds avatar Dec 30 '14 04:12 sds

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

camerond avatar Feb 10 '15 19:02 camerond

@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?

mdiebolt avatar Feb 23 '15 02:02 mdiebolt

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.

sds avatar Feb 23 '15 03:02 sds

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?

tbremer avatar Feb 24 '15 21:02 tbremer

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).

sds avatar Feb 24 '15 21:02 sds

Ahh, good point.

tbremer avatar Feb 24 '15 21:02 tbremer

@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.

mdiebolt avatar Feb 24 '15 22:02 mdiebolt

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

CREEATION avatar Feb 27 '15 09:02 CREEATION

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

batizhevsky avatar May 15 '15 13:05 batizhevsky

@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)

sds avatar Jun 02 '15 00:06 sds

+1

brunowego avatar Jul 03 '15 10:07 brunowego

yep, :+1:! @mdiebolt

CREEATION avatar Jul 03 '15 11:07 CREEATION

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.

bartveneman avatar Jul 04 '15 20:07 bartveneman

So... has this been resolved?

Plummat avatar Aug 10 '15 18:08 Plummat

+1 Wondering this as well for Atom

ryanmclaughlin avatar Aug 11 '15 03:08 ryanmclaughlin

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:

sds avatar Aug 11 '15 14:08 sds

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.

mdiebolt avatar Aug 11 '15 15:08 mdiebolt

+1

skrzyszewski avatar Sep 24 '15 23:09 skrzyszewski