Skip to content
This repository has been archived by the owner. It is now read-only.

Add Sass linting #30

Merged
merged 5 commits into from Feb 15, 2016
Merged

Add Sass linting #30

merged 5 commits into from Feb 15, 2016

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Feb 12, 2016

Begin using SCSS Lint to lint our style files:
https://github.com/brigade/scss-lint
https://github.com/brigade/scss-lint/blob/master/lib/scss_lint/linter/README.md

  • Decisions that vary from the default scss-lint config (which is a good default) are recorded in 954b564
  • CSS guidance in the style guide is quite minimal, based on these linting rules that should also be updated.
  • The rules should be a balance of legible, readable code and also highlighting the worst offences (eg highly nested rules)

Example output:

../government-frontend/app/assets/stylesheets/helpers/_available-languages.scss:23 [W] SingleLinePerSelector: Each selector in a comma sequence should be on its own single line
../government-frontend/app/assets/stylesheets/helpers/_available-languages.scss:27 [W] EmptyLineBetweenBlocks: Rule declaration should be followed by an empty line
../government-frontend/app/assets/stylesheets/helpers/_available-languages.scss:36 [W] EmptyLineBetweenBlocks: Rule declaration should be preceded by an empty line
../government-frontend/app/assets/stylesheets/helpers/_dash-list.scss:12 [W] EmptyLineBetweenBlocks: Rule declaration should be preceded by an empty line
../government-frontend/app/assets/stylesheets/helpers/_dash-list.scss:17 [W] SpaceAroundOperator: `$gutter-half*-1` should be written with a single space on each side of the operator: `$gutter-half *- 1`

Differences with rubocop:

  • Lacks diff only linting
  • Can't pass in multiple file directories

cc @dsingleton @tombye @gemmaleigh @benlovell

fofr added 5 commits Feb 12, 2016
* Hide options for disabled linters
* Turn off declaration order (eg @extend first), will be difficult to
fix
* Don’t care about Hex length for now – should use variables for the
most part
* There are some known good uses of !important, we don’t have an
excessive use of !important problem
* Don’t have opinions on .5em vs 0.5em
* Turn off property sort order, would create lots of noise to fix
* Turn off property spelling, new CSS features will give false positives
* Turn off property units, we advise on this in the style guide but the
advice isn’t extensive or adhered to
* Turn off pseudo element checks, older browsers use single colons
rather than double
* In qualifying element, allow the likes of `div[aria-hidden]`
* Disable SelectorDepth checks, we might want to enable this later, but
for reducing noise turn off for now
* Turn off StringQuotes checking, we might want to re-enable later
based on what would be reported
* Enable trailing zero checks (0.500em should be 0.5em)
* Enable transition all checks - bad for performance, unlikely we will
have this problem though
* Disable vendor prefix checks, we do this manually, perhaps we
shouldn’t
* Run from the specified config file
* Exclude vendor stylesheets from linting
If: `color: #43AC43` - this raises two warnings, incorrect hex case and
not using a variable. It’s preferable to show only the second error.
We want to reduce max_depth, but 3 is too low for many of the existing
styles. Using a depth of 5 and ignoring parent selectors will highlight
the worst offences.
@tijmenb
Copy link
Contributor

@tijmenb tijmenb commented Feb 12, 2016

Really cool 👍

Would it be possible to only specify the options where we deviate from the defaults in gds-sass-styleguide.yml? I've been looking into doing this for the rubocops too, so that it's easier to see where we differ, and we don't stray too much from community standards.

@dsingleton
Copy link
Contributor

@dsingleton dsingleton commented Feb 15, 2016

👏 to linting in general, and using govuk-lint makes this easy to roll out 👍

@fofr and I discussed the linting rules offline, and these are a good set of rules to start with, but we can always tighten them later. Not having a diff mode is less of an issue, as the amount of sass in apps tends to be much smaller than ruby and it's not too disruptive to mass-fix violations.

Only issue i've encountered is a bundler conflict when testing the gem with a frontend app:

Bundler could not find compatible versions for gem "sass":
  In snapshot (Gemfile.lock):
    sass (= 3.2.19)

  In Gemfile:
    govuk_frontend_toolkit (= 2.0.1) ruby depends on
      sass (>= 3.2.0) ruby

    sass-rails (~> 4.0.3) ruby depends on
      sass (~> 3.2.2) ruby

    govuk-lint (>= 0) ruby depends on
      scss_lint (~> 0.44.0) ruby depends on
        sass (~> 3.4.15) ruby

Running `bundle update` will rebuild your snapshot from scratch, using only
the gems in your Gemfile, which may resolve the conflict.

(Sadly bundle update doesn't resolve the issue for me)

I'm not a bundler expert, but that might make this awkward to use with some of our apps and we might need to massage the dependencies a bit - not 100% sure.

CC @benlovell or @boffbowsh for ruby-reckons?

@boffbowsh
Copy link
Contributor

@boffbowsh boffbowsh commented Feb 15, 2016

This is an issue with scss_lint itself. So even without this Gem wrapper we wouldn't be able to use the latest version of it due to conflicts. Boo hiss. Upgrade sass-rails in the app to fix it.

@fofr
Copy link
Contributor Author

@fofr fofr commented Feb 15, 2016

it's not too disruptive to mass-fix violations

Unlike Rubocop there is no auto-fix for violations, so though the files are fewer, the fix is a little more involved.

@fofr
Copy link
Contributor Author

@fofr fofr commented Feb 15, 2016

Would it be possible to only specify the options where we deviate from the defaults

I discussed this with @boffbowsh and we agreed that being explicit about what we think good is is better.

@dsingleton
Copy link
Contributor

@dsingleton dsingleton commented Feb 15, 2016

I was able to resolve the frontend app dependency issue with bundle update sass sass-rails. If the issue is likely to affect other frontend apps it'd be worth nothing in the CHANGELOG when bumping this gem.

Otherwise this LGTM, and i'd be happy with it being merged.

boffbowsh added a commit that referenced this pull request Feb 15, 2016
@boffbowsh boffbowsh merged commit 76fe222 into master Feb 15, 2016
1 check passed
1 check passed
default "Build #39 succeeded on Jenkins"
Details
@boffbowsh boffbowsh deleted the sass-linting branch Feb 15, 2016
fofr added a commit that referenced this pull request Feb 15, 2016
* Add sass linting using
[scss-lint](https://github.com/brigade/scss-lint) (#30)
  When upgrading you may have a bundle dependency issue. Running
`bundle update sass sass-rails` should fix it.
@fofr fofr mentioned this pull request Feb 15, 2016
fofr added a commit that referenced this pull request Feb 15, 2016
* Add sass linting using
[scss-lint](https://github.com/brigade/scss-lint) #30
* When upgrading you may have a bundle dependency issue. Running
`bundle update sass sass-rails` should fix it.
* Fix links to PRs in changelog
fofr added a commit that referenced this pull request Feb 15, 2016
* Add sass linting using
[scss-lint](https://github.com/brigade/scss-lint) #30
* When upgrading you may have a bundle dependency issue. Running
`bundle update sass sass-rails` should fix it.
@fofr fofr changed the title [DISCUSS] Add Sass linting Add Sass linting Feb 21, 2017
fofr added a commit to alphagov/government-frontend that referenced this pull request Jul 27, 2017
These classes are now allowed by our Sass linter:
alphagov/govuk-lint#30
fofr added a commit to alphagov/government-frontend that referenced this pull request Jul 28, 2017
These classes are now allowed by our Sass linter:
alphagov/govuk-lint#30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.