Adding scss-lint to the build


(Nick Schonning) #1

I briefly discussed this with @eviltrout last week, but I wanted to float this by @awesomerobot before I did any work. It does fall under the grey area of style related fixes, but I think it would benifit the project because it can be done once and then enforced going forward by Travis.

GitHub - brigade/scss-lint: Configurable tool for writing clean and consistent SCSS is a project to lint your SCSS code and ensure that it meets your coding conventions. You can take a look at the (growing) list of lints and chose which you would want disabled since it takes an on by default approach.


(Nick Schonning) #2

Took a quick pass without changing anything and came up with the following config to ignore all the currently flagged items

exclude:
  - "app/assets/stylesheets/vendor/**"

linters:
  StringQuotes:
    enabled: false
  Indentation:
    enabled: false
  Comment:
    enabled: false
  LeadingZero:
    enabled: false
  EmptyLineBetweenBlocks:
    enabled: false
  SelectorDepth:
    enabled: false
  CapitalizationInSelector:
    enabled: false
  NameFormat:
    enabled: false
  UrlFormat:
    enabled: false
  ZeroUnit:
    enabled: false
  BorderZero:
    enabled: false
  Shorthand:
    enabled: false
  PropertySortOrder:
    enabled: false
  SpaceAfterPropertyColon:
    enabled: false
  DuplicateProperty:
    enabled: false
  SingleLinePerSelector:
    enabled: false
  HexFormat:
    enabled: false
  SpaceBeforeBrace:
    enabled: false
  ColorKeyword:
    enabled: false
  DeclarationOrder:
    enabled: false
  TrailingSemicolonAfterPropertyValue:
    enabled: false
  SpaceAfterComma:
    enabled: false
  IdWithExtraneousSelector:
    enabled: false
  PlaceholderInExtend:
    enabled: false
  SpaceBetweenParens:
    enabled: false


(Kris) #3

I certainly don’t think that’s a bad idea. It’s been hard to stick to conventions as the core theme has been changing iteratively out of the original theme - so there’s a bit of a mix right now. It’s definitely been improving as things settle a bit towards 1.0, and I think a lint project could help.

I haven’t really had the chance to look at the specifics of SCSS-lint, but I don’t see much reason to not do it. Are there any?


(Nick Schonning) #4

The only “downside” is that is currently requires Sass 3.3, so it couldn’t be added to the directly to the project pipeline right now (seem to remember it was upgraded and then reverted).

I’ve setup on some other projects so that it only runs on Travis-CI, but can always be invoked with scss-lint . after installing the gem on your system. The problem with the local install approach is that new lints are enabled by default, so someone installing v0.22.0 today may pass (locally), but fail for someone else who gem install scss-lint later with a newer global gem install.


(Sam Saffron) #5

Update on this,

This PR is now closed and unmerged Scss linting by nschonni · Pull Request #2263 · discourse/discourse · GitHub

There are a few lessons here:

  • We should not attempt radical refactorings in a single PR. Instead we should work towards a goal.
  • I should have been on top of this a bit more, sorry
  • We really need sass-rails to work on the latest version of sass, this is a huge risk to us and all rails projects out there.