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 ignore: [single-declaration] to declaration-block-trailing-semicolon #5165

Merged
merged 2 commits into from Mar 6, 2021

Conversation

Mouvedia
Copy link
Contributor

@Mouvedia Mouvedia commented Feb 22, 2021

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

Closes #5159

Is there anything in the PR that needs further explanation?

It cannot be named "except" because it would require foo { property: value; } to be valid for "never" which is useless/stupid. Or do you imagine a use case for it that would warrant it?

Should it be supported just for consistency?
Can an option be exclusive to "always"? If so do you have an example?

@Mouvedia Mouvedia marked this pull request as draft February 22, 2021 19:38
@Mouvedia Mouvedia marked this pull request as ready for review February 22, 2021 19:54
@jeddy3
Copy link
Member

jeddy3 commented Feb 23, 2021

Thanks for the pull request.

Can an option be exclusive to "always"? If so do you have an example?

I can't think of one. If it's exclusive, then we've merged it into primary option. For example, "always-unless-keyword" in font-family-name-quotes.

Primary options and except secondary options avoid inconsistencies. For example: ignore would allow:

@keyframes bugfix { from { padding: 0 } to { padding: 0; } }

The ignore secondary option is usually used when another rule is going to enforce consistency instead, e.g. in a plugin.

Should it be supported just for consistency?

I think we support it for consistency, when catering to your use case.

And, just so I'm clear, you always want trailing semis except (i.e. never) when the declaration block as one declaration regardless of whether the block spans multiple lines?

Like so:

a { background: pink; color: red; }

a {
  background: pink;
  color: red;
}

a { color: red }
a {
  color: red
}

Or do your single declaration blocks always appear within single-line blocks?, e.g.:

a { color: red }

@Mouvedia
Copy link
Contributor Author

Mouvedia commented Feb 23, 2021

Primary options and except secondary options avoid inconsistencies. For example: ignore would allow:

@keyframes bugfix { from { padding: 0 } to { padding: 0; } }

Good point. Ill fix that.

Or do your single declaration blocks always appear within single-line blocks?

If you check the examples in the README you will see that it works for multi-line blocks as well.
I did it this way just not to complicate things further. Having this granularity may be added at a later time if someone demands it.

I think we support it for consistency, when catering to your use case.

What about { "singleDeclaration": "never" } instead? (ignored/harmless for :[ "never", {)
And if someone insists on having { "singleDeclaration": "always" } for :[ "never", { he will have to create another issue.

@Mouvedia Mouvedia marked this pull request as draft February 23, 2021 13:59
@Mouvedia Mouvedia marked this pull request as ready for review February 23, 2021 14:39
@Mouvedia Mouvedia changed the title Add ignore: ["single-declaration"] to declaration-block-trailing-semicolon (#5159) Add singleDeclaration: "never" to declaration-block-trailing-semicolon (#5159) Feb 23, 2021
@Mouvedia Mouvedia changed the title Add singleDeclaration: "never" to declaration-block-trailing-semicolon (#5159) Add singleDeclaration: "never"|"ignore-always" to declaration-block-trailing-semicolon (#5159) Feb 23, 2021
@jeddy3
Copy link
Member

jeddy3 commented Feb 24, 2021

Thanks for the answers and for updating the pull request.

I'd like to get a clear picture of your use case so that I'm sure we're going down the right path.

Would disallowing trailing semicolons inside single-line blocks cater to your use case, e.g:

a { color: red }
a { background: pink; color: red }

a {
  color: red;
}

This would also apply to the example you gave:

@keyframes bugfix { from { padding: 0 } to { padding: 0 } }

If so, then an except: ["inside-single-line-block"] secondary option, that inverts the primary option, would be consistent with other rules, e.g. declaration-empty-line-before, and, hopefully, easy to implement.

@Mouvedia
Copy link
Contributor Author

Mouvedia commented Feb 24, 2021

Would disallowing trailing semicolons inside single-line blocks cater to your use case

My personal use-case, would be single-line-block / ignore / for always only. That's not exactly what I have PRd though.

I added the consistent version which always require not having a trailing semicolon as { "singleDeclaration": "never" }.
And complemented it with { "singleDeclaration": "ignore-always" } which allows with and without trailing semicolon for single declarations.

…hopefully, easy to implement.

Like I said I didn't bother with single/multi line blocks for my second PR. It might be easier than I think; Ill have to check.

except: ["inside-single-line-block"]

That would require a { color: red; } to be valid for never which is undesirable/nonsense.

If it's exclusive, then we've merged it into primary option. For example, "always-unless-keyword" in font-family-name-quotes.

"always-unless-single-line-block-with-single-declaration" wouldn't solve my use case because we would still need to have two variants:

  • consistent/never
  • inconsistent/ignore

I'd like to get a clear picture of your use case

Ignore behaviour is desirable because sometimes you do know that this block may be expanded.
But the consistent case should also be available.

@jeddy3
Copy link
Member

jeddy3 commented Feb 24, 2021

Thanks, I think I understand now.

You don't add trailing semicolons when you're sure the block won't expand.

For example, in the case of:

@-webkit-keyframes bugfix { from { padding: 0 } to { padding: 0 } }

In that case, I think an ignore: ["inside-single-line-block"] option is best. Like other ignore options, it'll make the rule more permissive so that idiosyncratic patterns (or those connected to language extensions) can handled by hand or in a plugin.

If there are particular patterns for when a block won't ever be expanded, you can write a plugin to enforce the correct use of the trailing semicolon inside single-line blocks.

@Mouvedia
Copy link
Contributor Author

Mouvedia commented Feb 24, 2021

I think an ignore: ["inside-single-line-block"] option is best

I am alright with this but how do I convey that it's exclusive to always?
@jeddy3 Or do you want to evaluate a { color: red; } as also valid for :[ "never", { ignore: ["inside-single-line-block"] } ]?

["inside-single-line-block"]

There's another problem: it doesn't convey that it only matches if there's just one declaration.

@jeddy3
Copy link
Member

jeddy3 commented Feb 24, 2021

I am alright with this but how do I convey that it's exclusive to always?

It won't be exclusive to always. The rule will simply ignore single-line blocks when this option is used, regardless of the primary option.

@jeddy3 Or do you want to evaluate a { color: red; } as also valid for :[ "never", { ignore: ["inside-single-line-block"] } ]?

Yes, it is also valid.

There's another problem: it doesn't convey that it only matches if there's just one declaration.

It will ignore single-line blocks regardless of how many declarations there are. This will work for your use case, right?

The ignore option is used to make the rule more permissive. Trailing semicolons insde single-line blocks can then be handled by hand or via a plugin (which could take into account the number of declaration etc).

@Mouvedia
Copy link
Contributor Author

Mouvedia commented Feb 24, 2021

It will ignore single-line blocks regardless of how many declarations there are. This will work for your use case, right?

Actually, it's not. In my experience most of these once-set never-touch block declarations have a single declaration.
That's what I requested.
If it's somehow changed to mean always only for multi-line blocks this is a completely different requests and I am not interested in adding it.

Please check the README and tests and tell me what's wrong with it. I think what Iv come up with is a good compromise—even though it doesn't cater for single or multi line blocks.

@jeddy3
Copy link
Member

jeddy3 commented Feb 25, 2021

In my experience most of this once-set never-touch block declarations have a single declaration.

I think your original ignore: ["single-declaration"] will work best then.

Please check the README and tests and tell me what's wrong with it.

To keep the codebase sustainable, we tend to avoid adding code for idiosyncratic patterns. Instead, we add ignore options so that certain patterns are ignored by the built-in rules, and can then be checked by a complementary plugin, e.g. as how the stylelint-scss plugin does it.

@Mouvedia
Copy link
Contributor Author

I think your original ignore: ["single-declaration"] will work best then.

@jeddy3 done.

@Mouvedia Mouvedia changed the title Add singleDeclaration: "never"|"ignore-always" to declaration-block-trailing-semicolon (#5159) Add ignore: [single-declaration] to declaration-block-trailing-semicolon Feb 25, 2021
Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mouvedia Thanks for reverting to the ignore option. The code looks good.

I've requested some minor changes to the docs and tests to align them with our conventions and to be consistent with the rule's other examples and tests. The conventions help:

  • us maintain the test code
  • users when looking through lots of rules

Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM.

@jeddy3 jeddy3 mentioned this pull request Feb 26, 2021
6 tasks
Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left minor comments about the document, but this PR looks good to me. 👍

This was referenced Mar 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add except: ["single-declaration"] to declaration-block-trailing-semicolon
3 participants