Skip to content
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

Support Sass 3.5 #927

Merged
merged 5 commits into from
Nov 5, 2017
Merged

Support Sass 3.5 #927

merged 5 commits into from
Nov 5, 2017

Conversation

tagliala
Copy link
Contributor

@tagliala tagliala commented Nov 4, 2017

This is not the PR we deserve, but it's the one we need right now.

Sass 3.5 is supported, custom properties aren't: all specs are green (included the new one which deals with custom properties), and it does not fail against Bootstrap v4

Also:

  • Update development gems
  • Test against latest Ruby versions
  • Fix all new (0.50.0) RuboCop offences

Fix #877

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
Sass 3.4.20 contains a fix that allows SCSSLint to remove its `add_line_numbers_to_args` method

Ref: sass/sass@d6ea4c6
@tagliala tagliala mentioned this pull request Nov 4, 2017
@sds
Copy link
Owner

sds commented Nov 5, 2017

👏 🙇

@sds sds mentioned this pull request Nov 5, 2017
@tagliala
Copy link
Contributor Author

tagliala commented Nov 5, 2017

@sds

I want to submit a new PR that:

  • Move development dependencies to the proper place (gemspec). Take a look here: having them in the Gemfile lists them as runtime dependencies
  • Drop Ruby 2.0 support. This doesn't mean that Ruby 2.0 will not work anymore, but Ruby 2.1 will be required to develop the gem and we will exclude tests on Ruby 2.0, so we can also use RuboCop 0.51.0
  • Use coveralls_reborn, my (temporary, I hope) fork of coveralls, which seems unmaintained.

What do you think?

@sds
Copy link
Owner

sds commented Nov 5, 2017

Hey @tagliala, thanks for offering to move us forward on some of these issues.

  1. The reason we explicitly declared the RuboCop gem in the Gemfile was to avoid a warning about duplicate dependencies. See the commit message in 98655a7 for details. At its core, if another project includes SCSS-Lint as a development dependency, we don't want to force them to use the RuboCop version specified in the gemspec, since it's pinned to a specific version. If this isn't a practical issue, I'm open to suggestions here (is this behavior no longer the case?)

  2. I'm fine with dropping Ruby 2.0 support, as it's been EOLed since February 2016. I know your suggestion is to simply remove it from the Travis build matrix, but at that point we're not ensuring we support it, so I'd rather just drop support.

  3. Is there a summary of what exactly has changed with coveralls_reborn?

Thanks!

@tagliala
Copy link
Contributor Author

tagliala commented Nov 5, 2017

Hi,

  1. Got it, 👍
  2. Perfect, will do a separate PR just for this (after RuboCop releases a new version, because it has a lot of issues right now)
  3. It's not a big deal. It comes with updated development dependencies (mainly SimpleCov). I've submitted a couple of PRs in their repo three months ago, but it seems kinda unmaintained.
    Ref: diff, comment

@sds
Copy link
Owner

sds commented Nov 5, 2017

Thanks for linking to the diff for coveralls_reborn. The update to SimpleCov doesn't seem like it would benefit the SCSS-Lint project just yet (but if I'm missing something let me know) as none of the changes seem to affect this project.

If you find yourself down the road having an issue with the coverage information reported by SimpleCov, then I would be open to merging coveralls_reborn. My hope is that your PR eventually gets merged into the main project soon.

@tagliala
Copy link
Contributor Author

Perfect, will do a separate PR just for this (after RuboCop releases a new version, because it has a lot of issues right now)

RuboCop released a new version, but it has a lot of false positives, so I will keep waiting.

Moreover, Sass 3.5.4 has an issue (fixed in the stable branch) and a lot of tests with this gem fail sass/sass#2423, so I will wait for a new release of Sass, too

@tagliala tagliala mentioned this pull request Jan 8, 2018
@tagliala tagliala deleted the sass-35 branch January 11, 2018 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants