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

Add at-rule-semicolon-space-before rule #2388

Merged
merged 11 commits into from Feb 28, 2017

Conversation

4 participants
@CAYdenberg
Contributor

CAYdenberg commented Feb 23, 2017

Which issue, if any, is this issue related to?

#2346

Is there anything in the PR that needs further explanation?

skipBasicChecks is set to true in the first set of tests. The check @import "foo.css"; fails this test if the option is set to "always" (obviously).

@jeddy3 jeddy3 changed the title from Issue 2346 to Add at-rule-semicolon-space-before rule Feb 24, 2017

CAYdenberg and others added some commits Feb 25, 2017

@CAYdenberg

This comment has been minimized.

Contributor

CAYdenberg commented Feb 25, 2017

This is ready for review

@jeddy3

jeddy3 requested changes Feb 26, 2017 edited

This is looking very good, thanks!

Each new rule PR usually contains six files. The missing two in this instance are documentation: example config and the list of rules. Please can you add those.

@@ -0,0 +1,45 @@
# at-rule-semicolon-space-before
Require a single space or disallow whitespace before the semicolons of regular (non-nested) at-rules.

This comment has been minimized.

@jeddy3

jeddy3 Feb 26, 2017

Member

"Require a single space or disallow whitespace after the semicolons of at-rules."

There doesn't appear to be anything in the rule that avoids checking nested at-rules. Let's continue to check all at-rules to keep it consistent with at-rule-semicolon-newline-after, and just update this line in the docs.

This comment has been minimized.

@CAYdenberg

CAYdenberg Feb 27, 2017

Contributor

I think I'm failing lingo. I was trying to talk about at-rules like this one:

@font-face {
  font-family: "MyFont";
  src: url("myfont.woff2") format("woff2");
}

(I got the terminology from CSS Tricks)

These at rules are ignored because of the call to hasBlock, as is the case for at-rule-semicolon-newline-after.

Should I amend this to say "at-rules without declaration blocks" or some such?

This comment has been minimized.

@jeddy3

jeddy3 Feb 27, 2017

Member

I was trying to talk about at-rules like this one

Oh I see where you're coming from.

In the @font-face case those semi-colons belong to the "block of descriptor declarations", and not to the at-rule itself. In stylelint we generally generalise blocks like these as "declaration blocks".

Having said that...

Should I amend this to say "at-rules without declaration blocks" or some such?

... I don't think there's much need to be specific in the rule description as the examples quite clearly illustrate we're talking about semicolons after the likes of @import and @charset. I think "Require a single space or disallow whitespace after the semicolons of at-rules." will suffice here.

@import "components/buttons";
```
The following pattern is *not* considered a warning:

This comment has been minimized.

@jeddy3

jeddy3 Feb 26, 2017

Member

I'll update the valid and invalid example highlighting) on the website to accept these singular examples.

This comment has been minimized.

@CAYdenberg

CAYdenberg Feb 27, 2017

Contributor

Thanks.

}, {
code: "@import url(http://www.example.com/alocation;withsemicolon) ;",
}, {
code: "@import /*my styles;*/ \"styles/mystyle\" ;",

This comment has been minimized.

@jeddy3

jeddy3 Feb 26, 2017

Member

Can we get a multi-line accepted example please e.g.

@import 
  url('landscape.css') 
  projection ;

Same for "never".

This comment has been minimized.

@CAYdenberg

CAYdenberg Feb 27, 2017

Contributor

Addressed in 3a5e1cc

message: messages.expectedBefore(),
line: 1,
column: 26,
} ],

This comment has been minimized.

@jeddy3

jeddy3 Feb 26, 2017

Member

Can we get a multi-line rejected example please e.g.

@import 
  url('landscape.css') 
  projection;

Same for "never".

@jeddy3

Looking good. Thanks for adding the tests. I've two requests for the doc changes you made. Other than that this LGTM.

@@ -11,6 +11,7 @@ You might want to learn a little about [how rules are named and how they work to
"at-rule-empty-line-before": "always"|"never",
"at-rule-name-case": "lower"|"upper",
"at-rule-name-newline-after": "always"|"always-multi-line",
"at-rule-semicolon-space-before": "always"|"never",

This comment has been minimized.

@jeddy3

jeddy3 Feb 27, 2017

Member

Ordered alphabetically, please.

This comment has been minimized.

@CAYdenberg

CAYdenberg Feb 27, 2017

Contributor

My bad, got mixed up by the different newline rules. Addressed in e3f998d

@@ -238,6 +238,7 @@ Here are all the rules within stylelint, grouped by the [*thing*](http://apps.wo
- [`at-rule-no-unknown`](../../lib/rules/at-rule-no-unknown/README.md): Disallow unknown at-rules.
- [`at-rule-no-vendor-prefix`](../../lib/rules/at-rule-no-vendor-prefix/README.md): Disallow vendor prefixes for at-rules.
- [`at-rule-semicolon-newline-after`](../../lib/rules/at-rule-semicolon-newline-after/README.md): Require a newline after the semicolon of at-rules.
-[`at-rule-semicolon-space-before`](../../lib/rules/at-rule-semicolon-space-before/README.md): Require a single space or disallow whitespace before the semicolons of at rules.

This comment has been minimized.

@jeddy3

jeddy3 Feb 27, 2017

Member

You'll need a tab (or two) after the - for it to properly render.

@@ -0,0 +1,45 @@
# at-rule-semicolon-space-before
Require a single space or disallow whitespace before the semicolons of regular (non-nested) at-rules.

This comment has been minimized.

@jeddy3

jeddy3 Feb 27, 2017

Member

I was trying to talk about at-rules like this one

Oh I see where you're coming from.

In the @font-face case those semi-colons belong to the "block of descriptor declarations", and not to the at-rule itself. In stylelint we generally generalise blocks like these as "declaration blocks".

Having said that...

Should I amend this to say "at-rules without declaration blocks" or some such?

... I don't think there's much need to be specific in the rule description as the examples quite clearly illustrate we're talking about semicolons after the likes of @import and @charset. I think "Require a single space or disallow whitespace after the semicolons of at-rules." will suffice here.

@jeddy3

jeddy3 approved these changes Feb 28, 2017

Thanks for making the changes. LGTM.

@davidtheclark davidtheclark merged commit 9c7c38a into stylelint:master Feb 28, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Feb 28, 2017

Added to Changelog:

  • Added: at-rule-semicolon-space-before rule rulel (#2388).

@CAYdenberg CAYdenberg deleted the CAYdenberg:issue_2346 branch Feb 28, 2017

@CAYdenberg

This comment has been minimized.

Contributor

CAYdenberg commented Feb 28, 2017

Thanks everyone! Can I complement you on building on great OS community? The reviews have all been great, the docs are always clear, the scope of the project is always so well defined. Really great place to contribute.

sergesemashko pushed a commit to sergesemashko/stylelint that referenced this pull request Mar 3, 2017

Add at-rule-semicolon-space-before rule (stylelint#2388)
* Add README for at-rule-semicolon-space-before

* Wrote failing tests for at-rule-semicolon-space-before

* Add edge case tests

* Set up boilerplate for at-rule-semicolon-space-before

* Logic there - a few problems

* Check carefully column counts

* Set skipBasicChecks to true for the 'always' tests

* Add at-rule-semicolon-space-before to documentation

* Add some more multiline tests

* Amend documentation for at-rule-semicolon-space-before

m-allanson added a commit that referenced this pull request Mar 12, 2017

Merge branch 'master' into v8
* master:
  Update CHANGELOG.md
  Add flow comments annotation to function signatures (#2319)
  Update CHANGELOG.md
  Fix custom properties sets followed by comments in no-extra-semicolons (#2396)
  Fixed: `value-keyword-case` is now ignore counter, counters and attr fucntions.
  Fix typos (#2412)
  Fixed: `value-keyword-case` is now ignore counter-reset property.
  chore(package): update flow-bin to version 0.41.0
  chore(package): update eslint to version 3.17.0
  Fix the example for Custom Message in docs (#2409)
  Update CHANGELOG.md
  Update CHANGELOG.md
  Add at-rule-semicolon-space-before rule (#2388)
  Update CHANGELOG.md
  Print out filenames/globs when files do not exist (#2328)
  Tests: improved tests for `no-missing-end-of-source-newline` rule. (#2384)
  Update stylelint-disable-reason changelog entry (#2393)
  Add min rules
  chore(package): update flow-bin to version 0.40.0 (#2390)
  chore: Update `remark-*` modules (#2391)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment