Switch linting to stylelint #1875
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Aug 4, 2020
This was
linked to
issues
Aug 4, 2020
This was referenced Aug 4, 2020
Merged
vanitabarrett
force-pushed
the
stylelint-config-gds
branch
from
2fc97e2
to
f338a8d
Compare
kevindew
force-pushed
the
stylelint-config-gds
branch
from
f338a8d
to
31d7c2e
Compare
|
@vanitabarrett @hannalaakso I've updated this now with the inclusion of the declaration-no-important rule |
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-1875
vanitabarrett
approved these changes
Aug 26, 2020
kevindew
force-pushed
the
stylelint-config-gds
branch
from
31d7c2e
to
4e71395
Compare
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-1875
|
Thanks for the approval @vanitabarrett. I've now created the initial public release of stylelint-config-gds and rebased against this. Unfortunately I've learnt that there is a regression in the latest version of stylelint (which we depend on), which they're working on fixing quickly so I've rebased this with the failing branch. I'll ping you once this is sorted and this is ready to merge. I also noticed that earlier on you were experimenting with seeing about keeping some rules (I recall disallowing hsl function) do you want to consider raising an issue on stylelint-config-gds about those things if they're still a concern? |
|
Thanks for the update @kevindew 🙂 We're just digging into 448f78e to make sure we're happy with the media features being dropped in that commit, but all good otherwise. We decided against the other rules I was experimenting with (hsl and a stricter class regex for nesting), so nothing to raise against stylelint-config-gds there 👍 |
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-1875
This adds in a pre-release of stylelint-config-gds and disables all the rules in govuk-frontend that have violations. I chose to use a yaml file to define the configuration, rather than a JSON file or to use package.json, as I expect it'll need comments later
This rule forbids rogue empty lines at the end of blocks.
Fixing this violation also fixes the violation which block-closing-brace-newline-after was causing so this has also been removed.
The configuration provided by stylelint-config-standard is: https://stylelint.io/user-guide/rules/declaration-colon-newline-after#always-multi-line
The default rule (and convention) for this is to have a single space.
stylelint-config-standard uses https://stylelint.io/user-guide/rules/function-comma-newline-after#always-multi-line which requires all arguments in a multi-line function call to be on individual lines. Applying this conflicts with the sass-lint indention rule so this has been disabled in the places this has been applied.
stylelint-config-standard applies this always for multi-line rules: https://stylelint.io/user-guide/rules/function-parentheses-newline-inside#always-multi-line
This removes places where there are more than one empty line in a row. It originates from https://github.com/stylelint/stylelint-config-standard/blob/00d8b36bab00fb20426ca226338fccc2ee34f1f0/index.js#L67
This rule doesn't allow media features that aren't defined in CSS specifications (while making an exception for vendor prefixed ones). min-device-pixel-ratio never became an actual standard and the min-resolution rule is preferred. min--moz-device-pixel-ratio was a Mozilla specific rule that was dropped after Firefox 16 which is quite far out of the browser support range this project has. Orginates from: https://github.com/stylelint/stylelint-config-recommended/blob/59523c88fb1d83d922a3982c2ccf3c3c3b2776b3/index.js#L22
stylelint-config-standard [1] forbids this approach. [1]: https://github.com/stylelint/stylelint-config-standard/blob/00d8b36bab00fb20426ca226338fccc2ee34f1f0/index.js#L71
The violations of this rule appear to be not noticing an existing rule exists or a mistake.
These seem to be just developer mistakes.
This rule is defined in stylelint-config-standard [1] as:
```
"rule-empty-line-before": [
"always-multi-line",
{
except: ["first-nested"],
ignore: ["after-comment"],
},
],
```
These violations are fixes for where there is a new line provided for
the first nested rule.
[1]: https://github.com/stylelint/stylelint-config-standard/blob/00d8b36bab00fb20426ca226338fccc2ee34f1f0/index.js#L82-L88
The only file with a violation for this is font-face where the public CSS comment is intentional.
There was already a rule for this in scss:lint however the implementation there didn't seem to be sensitive enough to catch these violations.
This rule prevents using selectors that target elements not defined in HTML, SVG or MathML specs [1]. The only violation for this is an old fix for Firefox.
The stylelint-config-standard specifies lower case for values [1] [1]: https://github.com/stylelint/stylelint-config-standard/blob/00d8b36bab00fb20426ca226338fccc2ee34f1f0/index.js#L102
stylelint-config-standard allows 0 empty lines in a value list [1]. Since these violations appear to be added to break the content up into logical groupings I've added a disable rather than remove the new lines. [1]: https://github.com/stylelint/stylelint-config-standard/blob/00d8b36bab00fb20426ca226338fccc2ee34f1f0/index.js#L108
I've disabled the stylelint rule in device-pixels and spacing files as stylelint and sass-lint conflict over what they believe the indentation should be.
This enforces that psuedo elements are defined with a single colon. In the config/.sass-lint.yml file the rule for pseudo-elements was disabled which meant this project has a mix of both single colon and double colon pseudo elements. The rule to use single colon rather than double was requested by GOV.UK frontend developers as a means to not purposely break IE8 despite a lack of a official support, see alphagov/stylelint-config-gds#1 for more information.
This ports the rules from config/.sass-lint.yml [1] for ordering CSS properties. This ordering rule seems to be distinct to govuk-frontend and not adopted in other parts of GDS (as far as I'm aware) so I've set this rule as an extension specifically for govuk-frontend. There is probably a conversation to happen at some point as to whether this should be adopted wider or whether govuk-frontend wish to continue with it. [1]: https://github.com/alphagov/govuk-frontend/blob/2f73bd6d639db10c61d3b0da6f79f08150a4e530/config/.sass-lint.yml#L250-L405
This rule was accidentally missed in the initial config for stylelint-config-gds. This applies disabling of this rule in the situations where we make an exception and want important rules.
In stylelint 13.7.0 they now treat multiline SCSS comments as a single comment [1] which means the disabling techniques we previously used no longer work. By adopting this new syntax the reasons are also understood to be associated with the rule and can allow switching on the reportDescriptionlessDisables option [2] which can require the disabling of rules to always be documented. For example ``` // comment describing disabling // stylelint-disable indentation ``` is treated as a comment of: "comment describing disabling stylelint-disable indentation" Instead this can be resolved with the following technique: ``` // stylelint-disable indentation -- comment describing // disabling ``` This therefore updates all of the stylelint disabling of rules to match this pattern. [1]: stylelint/stylelint#4886
kevindew
force-pushed
the
stylelint-config-gds
branch
from
063d80e
to
dabd0a4
Compare
That's great @vanitabarrett - did you come to a decision on the commit (now ba6641d)? I've updated to stylelint 13.7.1 so this is now a passing build. Would it be helpful if I cherry picked #1902 onto this so it is just a single PR that goes in and applies both? |
|
Yay, thanks @vanitabarrett 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hello from GOV.UK 👋 , as forewarned this is a draft PR to propose a change in linter from the abandoned sass-lint to stylelint. This also abstracts the existing linting rules from this repo into a new package, stylelint-config-gds, which is intended to provide a stylelint configuration that can be recommended to all GDS projects via the GDS way.
Motivation for GOV.UK to work on this
GOV.UK is currently trying to migrate away from the deprecated Ruby SASS and instead use Sassc in projects. One of the blockers we have is that the linter we use, scss-lint, introduces Ruby SASS into projects and makes it cumbersome to migrate to Sassc.
We therefore want to upgrade our linter. We don't think GOV.UK needs to go it's own route with a linter and prefer our SCSS to be consistent with the design system, thus ideally we'd like to adopt a similar configuration as the design system. However since sass-lint is now abandoned it doesn't make much sense to put work into applying that to ~40 applications. Thus we felt it'd be beneficial to try extract the design system linting rules into a configuration and then see if that could be adopted across GDS. Stylelint felt like the obvious choice given it's ubiquity in CSS/SCSS linting.
The alternative approach - should this GDS attempt prove too complex, divisive or time consuming - is to have a stylelint configuration that is just for the GOV.UK programme and then to consider merging things at a later date.
stylelint-config-gds
We've put together a repository, stylelint-config-gds, that provides SCSS or CSS rules that can be applied to projects. The basic goals of this project were:
The approach taken involved building rules from stylelint-config-standard and then tweaking any rules where govuk-frontend's existing ruleset disagreed. These rules can be seen (and are annotated) in scss.js and css.js. One thing to note when reviewing these rules is that the stylelint project is renaming it's whitelist/blacklist rules in their next release to neutral terminology (I could understand this producing reservations in adopting this tool).
The project is named stylelint-config-gds to represent that this is intended for the GDS organisation. It may well be that we decide later we want to make this broader and then give it a different name. This seems like a future step unless anyone has clear ideas now.
Applying to govuk-frontend
The approach taken in this PR is to apply stylelint at the same time of scss-lint and then disable the occasional line/rule were there are minor disagreements between the linters. Although this is not a long term plan this did seem to offer a possible short term approach that two linters could run in parallel to aid adoption. I also broke down the applying of each rule to a single commit so that the effects of individual rules could be seen. It's likely easiest to review this PR commit by commit.
When establishing the rules for stylelint-config-gds it felt like the property ordering rule in govuk-frontend was somewhat more project specific than the other rules so I decided to employ that at a project level rather than in stylelint-config-gds. This was consistent to how GOV.UK adopted the scss-lint rules for one project but didn't use the property ordering rule. Do let me know if that raises eyebrows.
Anyway, here is a breakdown of rule differences from scss-lint:
bem-depth - there isn't an equivalent core stylelint, there is a potential plugin route but doesn't appear to be like-for-like. Could potentially also be achieved with specifying class name regex.
border-zero - An equivalent rule doesn't exists in stylelint-core as it's rather divisive topic. We could apply this by a plugin but I'm inclined to not have a rule for it given the divisive aspects.
class-name-format - there isn't the same BEM option available in stylelint core. Instead gone for a relatively loose regexp that's roughly similar.
no-color-literals - there isn't an equivalent rule in stylelint core that forces colors to always be hex. Could consider doing something with a regex if this is a concern.
no-important - rather than allowing important values I left stylelint to raise an error on an important. This feels like the sort of rule developers should have to disable themselves for a single declaration.
Next steps
The things we're looking for at this point are:
Do feel free to raise issues on https://github.com/alphagov/stylelint-config-gds too for more in-depth conversations about aspects than can be covered in this PR.
Thanks for your time :-) I'll look into arrange a meeting so we can chat more.