Skip to content
This repository has been archived by the owner on Mar 14, 2022. It is now read-only.

Replace scss-lint with sass-lint #268

Closed
aarmour opened this issue Jun 10, 2015 · 19 comments
Closed

Replace scss-lint with sass-lint #268

aarmour opened this issue Jun 10, 2015 · 19 comments
Labels
feature New feature request

Comments

@aarmour
Copy link

aarmour commented Jun 10, 2015

It's not ready yet, but with the switch to libsass it would be nice to eventually remove the Ruby dependency.

@kaelig
Copy link
Contributor

kaelig commented Jun 10, 2015

In the long term it's definitely something we need to do.

Right now it's nowhere near as mature as scss-lint, so feel free to contribute!

@Snugug
Copy link

Snugug commented Aug 20, 2015

Hey awesome people! We've got a few alpha releases out; all that's left for you to do is start contributing rules! We could use all the help we can get to roll this out quickly!

@kaelig
Copy link
Contributor

kaelig commented Aug 25, 2015

Hey Sam, thanks for coming here to see what we do :)

There is a plan to switch to sass-lint but it maybe not with such an early stage build. We've happily been guinea pigs on scss-lint but now that we're happy with our linting configuration I'm not sure we'll want to go through this again.

@Snugug
Copy link

Snugug commented Aug 25, 2015

Hey Kaelig!

The current build is stable, the alpha designator is simply because of the lack of rules (and some wonkiness with how config works from a yml file). The next release will most likely be a full BETA now that I've got the majority of rules written. If you could take a look and LMK what's missing from you adopting (or at least playing with adopting) it, that'd help me greatly get this out the door. I'm aiming for an early September launch presuming all of my testing/rule writing goes as planned.

@kaelig
Copy link
Contributor

kaelig commented Aug 25, 2015

Thank you for the update, I had no idea so many rules were already implemented.

I've ran a diff between the rules we enabled and the ones that are currently available in sass-lint:

-EmptyLineBetweenBlocks
-EmptyRule
+BangFormat
+BorderZero
+DebugStatement
+DeclarationOrder
+DuplicateProperty
+ElsePlacement
 FinalNewline
+HexLength
+HexNotation
+HexValidation
 IdSelector
-Indentation
+ImportPath
+ImportantRule
 LeadingZero
 NameFormat
 NestingDepth
 PlaceholderInExtend
-PropertySortOrder
 PropertySpelling
+QualifyingElement
 SelectorDepth
+SelectorFormat
 Shorthand
 SingleLinePerProperty
 SingleLinePerSelector
 SpaceAfterComma
 SpaceAfterPropertyColon
 SpaceAfterPropertyName
+SpaceBeforeBrace
 SpaceBetweenParens
-SpaceBeforeBrace
 TrailingSemicolon
+TrailingZero
+UnnecessaryMantissa
+VendorPrefix

Note that we're using an old version of scss-lint (0.35) so there might be old rules in there.

Hope it helps.

@Snugug
Copy link

Snugug commented Aug 25, 2015

Thanks!

@onishiweb
Copy link
Contributor

As mentioned in Financial-Times/ft-origami#448, JS scss-lint is now at v1 so we can start to look at implementing it now.

@JakeChampion
Copy link
Contributor

According to @kaelig's comment, we don't use DeclerationOrder (which was mentioned as the only blocker in the other issue).

@JakeChampion
Copy link
Contributor

Oh, we do, I seemed to have spelt it wrong ... DeclarationOrder not DeclerationOrder

@onishiweb
Copy link
Contributor

I'm going to double check that as I don't think we do actually use it. Not from what I've seen in the last version of the scss-lint file.

@onishiweb onishiweb added the feature New feature request label Mar 14, 2016
@ironsidevsquincy
Copy link
Contributor

It's definitely being used, unless I'm missing something

https://github.com/Financial-Times/origami-build-tools/blob/master/config/scss-lint.yml#L39-L40

@onishiweb
Copy link
Contributor

Nope, you're not. I didn't get chance to double check. I couldn't remember whether it was either not being used or just not a very strict order implemented. Will set to blocked until that rule is added

@Snugug
Copy link

Snugug commented Mar 14, 2016

Good news everyone! That's not an issue any more!

Sass Lint (the JS Sass style checker) is not a direct port of SCSS Lint, and takes a slightly different approach to rules, favoring small single-purpose rules over larger rules with lots of configuration (kinda like ESLint). We have two rules that replace the DeclarationOrder rules in SCSS Lint: extends-before-declarations and extends-before-mixins. There currently isn't a rule declarations-before-nesting, but if that's important to y'all A) it should be fairly straight forward to make and B) we'd be happy to include it.

@onishiweb
Copy link
Contributor

Awesome news, thanks @Snugug! I'll look to updating the scss-lint file and including Sass Lint over SCSS Lint in the near future.

In regards to the declarations-before-nesting it will probably be useful, so if it's straight forward and you're happy to add it, great. But also, if it's straight-forward we can always look at helping out too! Thanks sir.

@Snugug
Copy link

Snugug commented Mar 14, 2016

We're busy on our next release, but if you open a PR it's fairly easy for us to review those. Take a look at the code for those rules (~30 lines of actual code each, ignoring boilerplate), and if we can get it reviewed soon we may be able to get it out in our next release

https://github.com/sasstools/sass-lint/blob/develop/lib/rules/extends-before-declarations.js
https://github.com/sasstools/sass-lint/blob/develop/lib/rules/extends-before-mixins.js

@onishiweb
Copy link
Contributor

Opened PR on Sass Lint: sasstools/sass-lint#866

@onishiweb
Copy link
Contributor

@JakeChampion
Copy link
Contributor

PR for this #401

@JakeChampion
Copy link
Contributor

This was done in #401

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature New feature request
Projects
None yet
Development

No branches or pull requests

6 participants