Add support for Sass 3.5 #877

mprins · 2016-12-16T10:24:04Z

I'm looking at running scss_lint with the Sass 3.5 pre-release in my branch and the only issue I seem to be hitting is som failing testcases due to:

     NoMethodError:
       undefined method `to_sass' for ["1px solid #000"]:Array
       Did you mean?  to_s

(see travis builds)

Looking at the Sass API docs it seems the "proper name" is to_s but I might be overlookig something.
Is there an interest to have this adjusted (to_sass -> to_s)? and have things work on 3.5?

As a side note, but unrelated to this it seems that while the native rubies cause a failing build the jruby is green 😕

The text was updated successfully, but these errors were encountered:

sds · 2016-12-16T20:53:14Z

Hey @mprins, thanks for looking into running SCSS-Lint on Sass 3.5.

Looks like Travis lied to you, as the JRuby build definitely failed but reported as a pass. Looks like this is because while the RSpec tests failed the second step of our tests passed because we skip the Overcommit pre-commit checks for JRuby (since they aren't useful and usually fail).

In any case, would love a pull request that changes to_sass -> to_s if it results in all the tests passing. We'll probably want to modify our Travis build matrix to test on both Sass 3.4 and 3.5, if possible.

mprins · 2016-12-21T16:41:27Z

did a quick search-replace of to_sass -> to_s and ended up wit 60 or so test failures when using 3.5..; probably should start off with doing that code change first and try that with 3.4.x

In Sass 3.5, `node.value` returns an Array instead of a String. This could mean that we could expect multiple strings, so take this commit as an experiment. Fix: sds#877

tagliala · 2017-07-13T14:55:27Z

Hi,

I did an experiment in this commit: tagliala@7aeb1ff

All specs are green but I'm not confident of the solution and I will not submit a PR. Please read commit's message to understand what I changed and why.

There should be a reason for node.value to return an Array instead of a String (multiple strings) but I don't have time to investigate.

Anyway, hope it helps to speed up things

In Sass 3.5, `node.value` returns an Array instead of a String. This could mean that we could expect multiple strings, so take this commit as an experiment. Fix: sds#877

sds · 2017-07-15T16:01:56Z

Thanks for digging into this, @tagliala. Your general approach seems correct, but will not work in some cases. It seems Sass::Tree::PropNode#value now returns an array to represent interpolation in property values (see sass/sass@8247d55b0).

Thus the solution you suggest probably passes in tests because we don't have too many tests explicitly using interpolation in property values. We should add some tests and fix the implementations appropriately. In general, most of the linters should ignore the property value if it is composed of interpolations, since that is dynamic code that we can't easily evaluate in static analysis.

Would love to have someone dig into this and get this working, as it's not that difficult but just a bit tedious to ensure we're properly handling the Arrays returned by Sass::Tree::PropNode#value in all cases.

tagliala · 2017-09-23T08:38:01Z

Hi,

if you provide me some failing specs I could try to do a PR

apuntovanini · 2017-10-14T15:36:05Z

No updates on this? Bootstrap 4 now moved to Saas 3.5

sds · 2017-10-16T23:51:51Z

We're happy to accept a PR adding support for Sass 3.5! As mentioned above, this is doable (and @tagliala has a proof of concept that looks promising) but I think it's likely that the implementation will introduce some issues for codebases using interpolation in property values.

If someone applies the commit locally and runs SCSS-Lint against a sizable SCSS codebase (Bootstrap 4 is a good candidate) without issues, we'll be happy to take that as evidence that @tagliala's commit is sufficient and cut a release.

In Sass 3.5, `node.value` returns an Array instead of a String. This could mean that we could expect multiple strings, so take this commit as an experiment. Fix: sds#877

tagliala · 2017-10-17T09:31:55Z

@sds thanks for the hint.

I can confirm that my approach passes test but doesn't work on Bootstrap 4.

Here it is a failing css rule:

:root {
  --my-font-family: Helvetica;
}

Error: undefined method `visit_method' for String:Class

~/.rvm/gems/ruby-2.4.2/gems/sass-3.5.2/lib/sass/tree/visitors/base.rb:35:in `visit'
~/.rvm/gems/ruby-2.4.2/gems/sass-3.5.2/lib/sass/tree/visitors/set_options.rb:15:in `visit'
~/.rvm/gems/ruby-2.4.2/gems/sass-3.5.2/lib/sass/tree/visitors/base.rb:52:in `block in visit_children'
~/.rvm/gems/ruby-2.4.2/gems/sass-3.5.2/lib/sass/tree/visitors/base.rb:52:in `map'
~/.rvm/gems/ruby-2.4.2/gems/sass-3.5.2/lib/sass/tree/visitors/base.rb:52:in `visit_children'
~/.rvm/gems/ruby-2.4.2/gems/sass-3.5.2/lib/sass/tree/visitors/base.rb:36:in `block in visit'
~/.rvm/gems/ruby-2.4.2/gems/sass-3.5.2/lib/sass/tree/visitors/set_options.rb:92:in `visit_prop'
~/.rvm/gems/ruby-2.4.2/gems/sass-3.5.2/lib/sass/tree/visitors/base.rb:36:in `visit'
~/.rvm/gems/ruby-2.4.2/gems/sass-3.5.2/lib/sass/tree/visitors/set_options.rb:15:in `visit'
~/.rvm/gems/ruby-2.4.2/gems/sass-3.5.2/lib/sass/tree/visitors/base.rb:52:in `block in visit_children'
~/.rvm/gems/ruby-2.4.2/gems/sass-3.5.2/lib/sass/tree/visitors/base.rb:52:in `map'
~/.rvm/gems/ruby-2.4.2/gems/sass-3.5.2/lib/sass/tree/visitors/base.rb:52:in `visit_children'
~/.rvm/gems/ruby-2.4.2/gems/sass-3.5.2/lib/sass/tree/visitors/base.rb:36:in `block in visit'
~/.rvm/gems/ruby-2.4.2/gems/sass-3.5.2/lib/sass/tree/visitors/set_options.rb:35:in `visit_each'
~/.rvm/gems/ruby-2.4.2/gems/sass-3.5.2/lib/sass/tree/visitors/base.rb:36:in `visit'
~/.rvm/gems/ruby-2.4.2/gems/sass-3.5.2/lib/sass/tree/visitors/set_options.rb:15:in `visit'
~/.rvm/gems/ruby-2.4.2/gems/sass-3.5.2/lib/sass/tree/visitors/base.rb:52:in `block in visit_children'
~/.rvm/gems/ruby-2.4.2/gems/sass-3.5.2/lib/sass/tree/visitors/base.rb:52:in `map'
~/.rvm/gems/ruby-2.4.2/gems/sass-3.5.2/lib/sass/tree/visitors/base.rb:52:in `visit_children'
~/.rvm/gems/ruby-2.4.2/gems/sass-3.5.2/lib/sass/tree/visitors/base.rb:36:in `block in visit'
~/.rvm/gems/ruby-2.4.2/gems/sass-3.5.2/lib/sass/tree/visitors/set_options.rb:102:in `visit_rule'
~/.rvm/gems/ruby-2.4.2/gems/sass-3.5.2/lib/sass/tree/visitors/base.rb:36:in `visit'
~/.rvm/gems/ruby-2.4.2/gems/sass-3.5.2/lib/sass/tree/visitors/set_options.rb:15:in `visit'
~/.rvm/gems/ruby-2.4.2/gems/sass-3.5.2/lib/sass/tree/visitors/base.rb:52:in `block in visit_children'
~/.rvm/gems/ruby-2.4.2/gems/sass-3.5.2/lib/sass/tree/visitors/base.rb:52:in `map'
~/.rvm/gems/ruby-2.4.2/gems/sass-3.5.2/lib/sass/tree/visitors/base.rb:52:in `visit_children'
~/.rvm/gems/ruby-2.4.2/gems/sass-3.5.2/lib/sass/tree/visitors/base.rb:38:in `visit'
~/.rvm/gems/ruby-2.4.2/gems/sass-3.5.2/lib/sass/tree/visitors/set_options.rb:15:in `visit'
~/.rvm/gems/ruby-2.4.2/gems/sass-3.5.2/lib/sass/tree/visitors/set_options.rb:5:in `visit'
~/.rvm/gems/ruby-2.4.2/gems/sass-3.5.2/lib/sass/tree/node.rb:102:in `options='
~/.rvm/gems/ruby-2.4.2/gems/sass-3.5.2/lib/sass/engine.rb:420:in `_to_tree'
~/.rvm/gems/ruby-2.4.2/gems/sass-3.5.2/lib/sass/engine.rb:321:in `to_tree'
~/dev/scss-lint/lib/scss_lint/engine.rb:35:in `initialize'
~/dev/scss-lint/lib/scss_lint/runner.rb:31:in `new'
~/dev/scss-lint/lib/scss_lint/runner.rb:31:in `find_lints'
~/dev/scss-lint/lib/scss_lint/runner.rb:19:in `block in run'
~/dev/scss-lint/lib/scss_lint/runner.rb:18:in `each'
~/dev/scss-lint/lib/scss_lint/runner.rb:18:in `run'
~/dev/scss-lint/lib/scss_lint/cli.rb:67:in `scan_for_lints'
~/dev/scss-lint/lib/scss_lint/cli.rb:59:in `act_on_options'
~/dev/scss-lint/lib/scss_lint/cli.rb:33:in `run'
~/dev/scss-lint/bin/scss-lint:7:in `<top (required)>'
~/.rvm/gems/ruby-2.4.2/bin/scss-lint:23:in `load'
~/.rvm/gems/ruby-2.4.2/bin/scss-lint:23:in `<main>'
~/.rvm/gems/ruby-2.4.2/bin/ruby_executable_hooks:15:in `eval'
~/.rvm/gems/ruby-2.4.2/bin/ruby_executable_hooks:15:in `<main>'

Apparently, there are issues with custom properties. Master branch works fine.

This is the only issue while linting Bootstrap 4

I've also rebased my branch with the latest master release: tagliala@7aeb1ff

Hope that someone else could finish my job

tagliala · 2017-10-21T00:29:10Z

Bundler could not find compatible versions for gem "sass":
  In Gemfile:
    bootstrap (~> 4.0.0.beta2) was resolved to 4.0.0.beta2, which depends on
      sass (>= 3.5.2)

    scss_lint (~> 0.55.0) was resolved to 0.55.0, which depends on
      sass (~> 3.4.20)

The time has come 😐

tagliala · 2017-10-22T11:08:17Z

Ref:

tagliala · 2017-10-22T12:02:05Z

I have a failing spec for engine_spec.rb

  context 'when a custom property is present' do
    let(:scss) { <<-SCSS }
      :root {
        --my-font-family: Helvetica;
      }
    SCSS

    it 'has a parse tree' do
      engine.tree.should_not be_nil
    end
  end

and a clue. If you simply add a new line with stylesheet before this line: https://github.com/sass/sass/blob/3.5.2/lib/sass/scss/parser.rb#L41

Spec will pass

Sorry, I cannot invest more time on this

Git bisect result on Sass: sass/sass@8247d55

In Sass 3.5, `node.value` returns an Array instead of a String. This could mean that we could expect multiple strings, so take this commit as an experiment. Also, Sass 3.5.3 fixes a couple of issues which previously need a monkey patch in scss-lint Fix: sds#877 Ref: sass/sass#1799, sass/sass#2394

In Sass 3.5, `node.value` returns an Array instead of a String. This could mean that we could expect multiple strings. With this PR, SCSS-Lint will check the first string returned. Also, Sass 3.5.3 fixes a couple of issues which previously need a monkey patch in SCSS-Lint. This PR uses '~> 3.5.3' as the minimum required version of Sass Fix: sds#877 Ref: sass/sass#1799, sass/sass#2394

tagliala · 2017-10-27T13:08:20Z

Hi,

Sass released 3.5.3, with a fix that positively affects SCSS-Lint. I've update my commit with a failing spec. sass/sass#2394

At the moment the error is generated inside the Sass library, which is attempting to call the visit_method method on a string. I'm still unable to understand if the problem is in SCSS-Lint or in Sass itself.

Any help would be hugely appreciated

PS: I know I should give up on Ruby and go for JavaScript/node, but I will not do that 🤷‍♂️

In Sass 3.5, `node.value` returns an Array instead of a String. This could mean that we could expect multiple strings. With this PR, SCSS-Lint will check the first string returned. Also, Sass 3.5.3 fixes a couple of issues which previously need a monkey patch in SCSS-Lint. This PR uses '~> 3.5.3' as the minimum required version of Sass Fix: sds#877 Ref: sass/sass#1799, sass/sass#2394

sds changed the title ~~running with Sass 3.5~~ Add support for Sass 3.5 Jun 4, 2017

sds added the enhancement label Jun 4, 2017

tagliala added a commit to tagliala/scss-lint that referenced this issue Jul 13, 2017

Support Sass 3.5

5ab8376

In Sass 3.5, `node.value` returns an Array instead of a String. This could mean that we could expect multiple strings, so take this commit as an experiment. Fix: sds#877

tagliala added a commit to tagliala/scss-lint that referenced this issue Jul 13, 2017

Support Sass 3.5

bce009d

In Sass 3.5, `node.value` returns an Array instead of a String. This could mean that we could expect multiple strings, so take this commit as an experiment. Fix: sds#877

sds mentioned this issue Aug 5, 2017

grid-template-columns: [ name ] throws a syntax error #906

Closed

tagliala added a commit to tagliala/scss-lint that referenced this issue Oct 17, 2017

Support Sass 3.5

7aeb1ff

In Sass 3.5, `node.value` returns an Array instead of a String. This could mean that we could expect multiple strings, so take this commit as an experiment. Fix: sds#877

mprins mentioned this issue Oct 27, 2017

Next release (3.5?) GeoDienstenCentrum/sass-maven-plugin#163

Closed

tagliala mentioned this issue Oct 28, 2017

Release sass dependency #923

Closed

tagliala mentioned this issue Nov 4, 2017

Support Sass 3.5 #927

Merged

sds closed this as completed in #927 Nov 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Sass 3.5 #877

Add support for Sass 3.5 #877

mprins commented Dec 16, 2016

sds commented Dec 16, 2016

mprins commented Dec 21, 2016

tagliala commented Jul 13, 2017 •

edited

Loading

sds commented Jul 15, 2017 •

edited

Loading

tagliala commented Sep 23, 2017

apuntovanini commented Oct 14, 2017

sds commented Oct 16, 2017

tagliala commented Oct 17, 2017 •

edited

Loading

tagliala commented Oct 21, 2017

tagliala commented Oct 22, 2017 •

edited

Loading

tagliala commented Oct 22, 2017 •

edited

Loading

tagliala commented Oct 27, 2017

Add support for Sass 3.5 #877

Add support for Sass 3.5 #877

Comments

mprins commented Dec 16, 2016

sds commented Dec 16, 2016

mprins commented Dec 21, 2016

tagliala commented Jul 13, 2017 • edited Loading

sds commented Jul 15, 2017 • edited Loading

tagliala commented Sep 23, 2017

apuntovanini commented Oct 14, 2017

sds commented Oct 16, 2017

tagliala commented Oct 17, 2017 • edited Loading

tagliala commented Oct 21, 2017

tagliala commented Oct 22, 2017 • edited Loading

tagliala commented Oct 22, 2017 • edited Loading

tagliala commented Oct 27, 2017

tagliala commented Jul 13, 2017 •

edited

Loading

sds commented Jul 15, 2017 •

edited

Loading

tagliala commented Oct 17, 2017 •

edited

Loading

tagliala commented Oct 22, 2017 •

edited

Loading

tagliala commented Oct 22, 2017 •

edited

Loading