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

Move from Ruby scss-lint to Node sass-lint #7620

Closed
gakimball opened this issue Dec 18, 2015 · 19 comments
Closed

Move from Ruby scss-lint to Node sass-lint #7620

gakimball opened this issue Dec 18, 2015 · 19 comments
Assignees

Comments

@gakimball
Copy link
Contributor

There's now a Node-powered Sass linting library, appropriately called sass-lint. Right now we're using gulp-scss-lint, which is a wrapper around the Ruby-based scss-lint.

Let's migrate our linting config file to sass-lint and drop that Ruby dependency.

@colin-marshall
Copy link
Contributor

I'm on it.

@colin-marshall
Copy link
Contributor

Using this: http://sasstools.github.io/make-sass-lint-config/

It came back that these are not supported yet in sass-lint:

# The following scss-lint Linters are not yet supported by sass-lint:
# ElsePlacement, PropertyCount, SelectorDepth, UnnecessaryParentReference
# VendorPrefixes, Compass::*
# 
# The following settings/values are unsupported by sass-lint:
# Linter Indentation, option "allow_non_nested_indentation"
# Linter Indentation, option "character"
# Linter PropertySortOrder, option "separate_groups"
# Linter SpaceBeforeBrace, option "allow_single_line_padding"

Should I still proceed or should we wait until these are added to sass-lint?

@gakimball
Copy link
Contributor Author

Hmm, I like SelectorDepth, but maybe if a big open source project adopts sass-lint we can press them to add more linters ;)

Go ahead and keep working on the implementation. I'll do some linting with the existing file today just so everything is up-to-date.

@colin-marshall
Copy link
Contributor

I know they plan on adding PropertyCount: sasstools/sass-lint#406

I will open an issue over there and see what's up.

@colin-marshall
Copy link
Contributor

@gakimball: We got a great response from @DanPurdy: sasstools/sass-lint#457 (comment)

I can open up some new issues over there for specific Linter requests. Besides SelectorDepth are there any other important ones you would like before the switch can happen?

@gakimball
Copy link
Contributor Author

We can do the switch right now and I'll keep tabs on the library as it develops.

@gakimball
Copy link
Contributor Author

Also it looks like nesting-depth covers what SelectorDepth is doing.

@colin-marshall
Copy link
Contributor

There is a problem with making the switch right now. sass-lint throws a parse error when there is a string interpolated on a CSS property so it doesn't complete the linting process. I opened a new issue on the gonzales-pe repo to get this fixed: tonyganch/gonzales-pe#140

@gakimball
Copy link
Contributor Author

Thanks man! Loving your dedication :)

Have you gotten any swag from us? Email me at geoff@zurb.com your home address and I can hook you up in the next few weeks.

@colin-marshall
Copy link
Contributor

No problem! Glad I can help out on an amazing project like Foundation :)

Rafi shot me an email earlier this month asking for my address to send me a t-shirt. Looking forward to receiving it. Thanks @gakimball!

@SalahAdDin
Copy link

👍

@colin-marshall
Copy link
Contributor

Update: I think when the PR sasstools/sass-lint#459 gets merged we should be able to use this.

@gakimball
Copy link
Contributor Author

@colin-marshall Looks like that issue was merged! I'm itching to remove this Ruby dependency :) You want to finish the integration?

@colin-marshall
Copy link
Contributor

@gakimball it was more complicated than I initially thought. That PR was merged but now instead of breaking during linting when it comes across an SCSS file with string interpolation, it gives an error and processes the next file. So close to half of the files aren't linted at all and show a fatal error.

The AST sass-lint uses (gonzales-pe) fixed the interpolation issue for us (tonyganch/gonzales-pe#140), but sass-lint is using an older version of gonzales-pe and is nearly ready to update to the latest version. I was waiting until the issue sasstools/sass-lint#495 is resolved which will allow sass-lint to use the latest version of gonzales-pe. It should be any day now.

I'll put in a PR tomorrow so you can see if you want to merge it as is or wait until sass-lint gets updated to 1.6.0.

@DanPurdy
Copy link

Hi guys, yeah just to add that the delay in 1.6 is currently down to me updating the last rule to the new gonzales structure. I'm hoping to find some time this week / weekend to update it and then 1.6 should be released. I would hold off until 1.6 is out as it does fix a lot of the main issues we have with parsing files etc.

We'd like to see you update as much you! 😄

@gakimball
Copy link
Contributor Author

@DanPurdy Awesome, thanks for the update :)

@colin-marshall
Copy link
Contributor

@DanPurdy thanks for the hard work you guys have been putting in to get the update out!

@colin-marshall
Copy link
Contributor

Update: Shout out to @DanPurdy, @bgriffith, and the @sasstools team for their hard work to finish and release v1.6 of sass-lint. I just added 1.6 to my node-sass-lint branch and lots of files that were throwing fatal errors are now being linted. We're still experiencing fatal errors with 6 of our files, so I'm going to hold off on merging my branch until we can get the rest of those linting.

I've opened these 2 issues on sass-lint:
sasstools/sass-lint#643
sasstools/sass-lint#644

And @DanPurdy opened this one up for me on https://github.com/tonyganch/gonzales-pe:
tonyganch/gonzales-pe#169

colin-marshall added a commit that referenced this issue Nov 9, 2016
Migrate from scss-lint to sass-lint (#7620)
@colin-marshall
Copy link
Contributor

Merged and closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants