Skip to content
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 except: ["after-closing-brace"] to block-closing-brace-empty-line-before #2090

Closed
thomasjbradley opened this issue Nov 21, 2016 · 14 comments
Labels
good first issue is good for newcomers status: ready to implement is ready to be worked on by someone type: new option a new option for an existing rule

Comments

@thomasjbradley
Copy link
Contributor

Describe the issue. Is it a bug or a feature request (new rule, new option, etc.)?

Possibly a bug, but most likely a feature request.

I'd like to be able to force an empty line at the end of a media query—we can do it at the top of a media query, but I can't seem to find how to do it at the bottom.

@media print {

  a {
    color: red;
  }
            /* <----- Force this empty line here */
}

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

I see the rule: rule-nested-empty-line-before which will force a new line at the top of the media query.

And I see the rule: block-closing-brace-empty-line-before which will force a new line at the end of the block, but that would also force a new line after color: red.

What CSS is needed to reproduce this issue?

This CSS should pass:

@media print {

  a {
    color: red;
  }

}

This CSS should fail:

@media print {

  a {
    color: red;
  }
}

Which version of stylelint are you using?

Currently using 7.5.0.

How are you running stylelint: CLI, PostCSS plugin, Node API?

Node API.

Does your issue relate to non-standard syntax (e.g. SCSS, nesting, etc.)?

Nope.

What did you expect to happen?

I'd like to get a warning message if there isn’t an empty line at the end of a media query.

What actually happened (e.g. what warnings or errors you are getting)?

Both empty lines and non empty lines pass Stylelint tests.

I feel like an older version used to enforce this, but looking through past commits of my Stylelint config I don't see anything that jumps out at me as being “the rule” that enforced that requirement.

@ntwb ntwb added the status: needs discussion triage needs further discussion label Nov 22, 2016
@ntwb
Copy link
Member

ntwb commented Nov 22, 2016

Thanks for using our issue template and the detailed description @thomasjbradley 👍

p.s. Originally via this Stack Overflow issue.

@jeddy3
Copy link
Member

jeddy3 commented Nov 23, 2016

I think we can achieve this with the addition of an exceptAtRules: [] option to block-closing-brace-empty-line-before.

There is a precedence for *-empty-line-before rules accepting user defined options in at-rule-empty-line-before. And one for block-closing-brace-* rules in block-closing-brace-newline-after.

This would be the first instance of a user defined except*: [] option though, but I can't foresee any issues with such an option type.

So, the following config:

"block-closing-brace-empty-line-before": ["never", { "exceptAtRules": ["media"] }]

Would enforce this:

@media print {

  a {
    color: red;
  }

}

Having an user defined option, rather than just the keyword one of except: ["media"], provides flexibility for applying a similar code style to other at-rules e.g. @supports:

"block-closing-brace-empty-line-before": ["never", { "exceptAtRules": ["media", "supports"] }]
@media print {

  a {
    color: red;
  }

}

@supports (display:flex) {

  a {
    display: flex;
  }

}

Unless there are objections, I'll label this up as "help wanted" and point @thomasjbradley in the direction of how to contribute this option.

@davidtheclark
Copy link
Contributor

@jeddy3 What if someone wants to use the exception for all at-rules?

@jeddy3
Copy link
Member

jeddy3 commented Nov 23, 2016

What if someone wants to use the exception for all at-rules?

Oops, I hadn't thought about that.

@thomasjbradley Is your code style to have an empty line before the closing brace of just @media or all at-rules e.g. @support, @page, @font-face, or do you have one code style for at-rules that nest rule-sets (e.g. @media, @keyframes etc), but not for those that only contain declarations e.g. @font-face?

That last one makes me think that having the flexibility to specify exactly which at-rules are exceptions is desirable, but I'm 100% sure now.

@thomasjbradley
Copy link
Contributor Author

For me it would only be nested rulesets. So @supports, @keyframes, @media — but not @font-face.

@jeddy3
Copy link
Member

jeddy3 commented Nov 23, 2016

@davidtheclark So it sounds like could have an except: ["nesting-at-rules"] keyword option, but I'm not in favour of that because it doesn't account for custom at-rules e.g.

@my-custom-nesting-at-rule {

  a {}

}

The exceptAtRules: ["media", "supports", "keyframes", "my-custom-nesting-at-rule"] option does require more configuration, but it feels necessary for this flexibility.

@davidtheclark
Copy link
Contributor

it doesn't account for custom at-rules

Why not? If we're just checking that the at-rule contains rules, that would apply just as much to custom at-rules as standard ones, right?

@jeddy3
Copy link
Member

jeddy3 commented Nov 23, 2016

I realised the same thing on the train. I originally thought we'd have to keep a list of standard nesting at-rules, but now, like you, I see that we can just detect if the at-rule contains rules.

except: ["nesting-at-rules"] works for me.

@jeddy3
Copy link
Member

jeddy3 commented Nov 26, 2016

I'll label this issue up accordingly.

@thomasjbradley Would you like to contribute this option? rule-non-nested-empty-line-before has some examples of how such an except option might work. In this instance, you'll want to check if the atRule node contains any atRule or rule children and then invert the primary option if it does.

@jeddy3 jeddy3 changed the title Force a new line before a media query’s closing brace Add except: ["nesting-at-rules"] option to block-closing-brace-empty-line-before Nov 26, 2016
@jeddy3 jeddy3 added status: ready to implement is ready to be worked on by someone type: new option a new option for an existing rule and removed status: needs discussion triage needs further discussion labels Nov 26, 2016
@davidtheclark davidtheclark added the good first issue is good for newcomers label Nov 26, 2016
@jeddy3
Copy link
Member

jeddy3 commented Dec 13, 2016

For me it would only be nested rulesets

In line with #1663, I'm pretty sure this option should be "after-closing-brace".

@jeddy3 jeddy3 changed the title Add except: ["nesting-at-rules"] option to block-closing-brace-empty-line-before Add except: ["after-closing-brace"] option to block-closing-brace-empty-line-before Dec 13, 2016
@jonathantneal
Copy link

jonathantneal commented Jan 11, 2017

I would like to define an exception for block-closing-brace-newline-after. Is this possible now, or am I just seconding this request? My use-case is:

{
  "block-closing-brace-newline-after": ["always", { "exceptAtRules": ["else"] }]
}
@media (width < 500) {
  display: none;
} @else {
  display: block;
}

Citations:
https://tabatkins.github.io/specs/css-when-else/#else-rule
https://github.com/jonathantneal/postcss-at-else

@jeddy3
Copy link
Member

jeddy3 commented Jan 11, 2017

Is this possible now

@jonathantneal It is possible now using a couple of rules from stylelint-scss (a plugin pack for SCSS-like constructs). Those guys wrote up an example on how to do it.

or am I just seconding this request

The request in this issue is for something different i.e. having an empty line before a closing brace if it is preceded by another closing brace.

@ArmorDarks
Copy link

ArmorDarks commented Jul 19, 2017

Maybe we need even more generic approach, which could be used almost with any Stylelint rule?

For instance, we can determinate interface, which will allow to target within ignore and except any recognized by Stylelint CSS entity and whenever needed, to specify which exactly instance of entity should it be.

It could look like

except:
- property
- at-rule:
  - media
  - blockless # target blockless at-rules
  - same-name # target entities with same name
  - single-line # reverse behavior for any entity represented as single line
- ruleset
- whatever_else_Stylelint_recognizes
ignore:
- property
- at-rule:
  - 'media'
- ruleset
- whatever_else_Stylelint_recognizes

This interface can be applicable to any rule, which evaluates based on whether or not something appears before or after in AST.

It will also neglect some strange situations, when some rules have specific ignores and excepts, while other no, or ignores and excepts are different not because they aren't useful, but simply because they weren't foresighted during development.

@jeddy3 jeddy3 changed the title Add except: ["after-closing-brace"] option to block-closing-brace-empty-line-before Add except: ["after-closing-brace"] to block-closing-brace-empty-line-before Sep 10, 2017
@ntwb ntwb removed the Hacktoberfest label Nov 4, 2017
@ArmorDarks
Copy link

It is kinda strange when here #2090 (comment) and here #2090 (comment) we were talking about some nicer, more generic and broad interface, and than issue suddenly closed by slightly PR, which solves some problems, but obviously doesn't work as a solution for author's original issue with enforcing empty line only in at rules, while the PR allows doing so only globally

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue is good for newcomers status: ready to implement is ready to be worked on by someone type: new option a new option for an existing rule
Development

No branches or pull requests

7 participants