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
Fix regression for disable commands and adjacent double-slash comments #4937
Comments
As comment merging is useful only for descriptions of disable descriptions, I propose the following heuristic:
An alternative to the second rule could be to forbid merging a comment starting with |
@stof Thanks for writing this up.
We wanted to merge double-slash comment blocks so that the behaviour is consistent with the multiline CSS comments. (It's also how the SCSS parser works, and postcss-scss will likely mimic this behaviour in the future.) Multiline comments are useful when a user is disabling many rules (as well as now being useful for disable descriptions). The following has always worked in stylelint: /*
stylelint-disable unit-allowed-list, media-feature-name-disallowed-list
color-no-hex, property-no-unknown
*/ The change was to bring double-slash comment blocks in line with this behaviour: // stylelint-disable unit-allowed-list, media-feature-name-disallowed-list
// color-no-hex, property-no-unknown The change would also benefit the new disables description feature: /*
stylelint-disable unit-allowed-list, media-feature-name-disallowed-list
color-no-hex, property-no-unknown
-- description
*/
// stylelint-disable unit-allowed-list, media-feature-name-disallowed-list
// color-no-hex, property-no-unknown
// -- description In hindsight, it's clear that the merging of double-slash comment blocks was a breaking change. It changes the behaviour for: // before
// stylelint-disable color-no-hex
a { color: #fff; } And: // stylelint-disable color-no-hex
// after
a { color: #fff; } Previously both those stylelint disable commands would work but now they don't. Demo. I'm not sure there's a heuristic we can add to fix the latter example. What's the normal play when a breaking change is released as a patch? Do we revert the change, release a patch and then prepare for a major release with the change in? |
Usually that's the procedure indeed. |
my proposal here would fix the regression for the case with text before it (which currently gets silently ignored as not being a |
Note that the breaking change was released in a minor, not in a patch. 13.7.1 contains a partial fix for the regression. |
Yes, I think your proposal would work for that regression.
It doesn't report that the rule name is not valid. It silently breaks the disable command, and behaves in the same away as the case with text before. We could change the behaviour to report it as an invalid name, but that would also be a breaking change and should be a major release. I can't think of a heuristic that would work around this in a non-breaking way, especially when we consider the Can anyone think of a non-breaking solution to: // stylelint-disable color-no-hex
// after
a { color: #fff; } |
Following condition should be added:
It's all according to initial discussion #4886. I noticed that we don't have tests for following cases for /*
stylelint-disable block-opening-brace-space-after
--
after
*/ /*
stylelint-disable block-opening-brace-space-after
-- after
*/ // stylelint-disable block-opening-brace-space-after -- after // stylelint-disable block-opening-brace-space-after
// -- after
I don't think so. Demo. |
It would be unfortunate, because in CSS We have 3 separate comment: /* a */
/* stylelint-disable */
/* b */ If we convert them to Sass: // a
// stylelint-disable
// b And then back to CSS (virtually, it just easier to understand what grouping of comments would do in /* a
stylelint-disable
b */ This is completely different thing, which is not important for Sass compiler, but is important for stylelint, Autoprefixer, Prettier and other tools, which use control comments. |
the rule is more complex than that:
This way, merging also works for these cases: // stylelint-disable block-opening-brace-space-after
// --
// after with
// multiline description // stylelint-disable block-opening-brace-space-after -- after with
// multiline description // stylelint-disable block-opening-brace-space-after
// stylelint-enable other-rule |
By the way, we have tests for this, but we don't have documentation for this behavior. It's undocumented feature, which we could safely remove, I think.
Good one! I forgot about enable comment :) |
/*
stylelint-disable unit-allowed-list, media-feature-name-disallowed-list
color-no-hex, property-no-unknown
*/
My bad, I missed a comma. Demo of it working. /*
stylelint-disable block-opening-brace-space-after,
color-hex-case
*/
a {color: #FFF; } The intent of the original issue was to add support for multiline descriptions for double slash comments. It did this by merging adjacent comments, which also enabled: // stylelint-disable block-opening-brace-space-after,
// color-hex-case
a {color: #FFF; } Demo. But broke: // stylelint-disable block-opening-brace-space-after
// after
a {color: #FFF; } Demo.
I agree that it's a different behaviour. I also agree with the expectation from the original issue, though:
i.e. adjacent double-slash comments are consistently treated as a single unified comment regardless of the content within them. I think this behaviour is useful for control comments, e.g.: // stylelint-disable block-opening-brace-space-after,
// color-hex-case, color-no-invalid-hex
// -- hacky code description
a {color: #FF; } Demo. Users will need to write: // a
// stylelint-disable block-opening-brace-space-after,
// color-hex-case, color-no-invalid-hex
// -- hacky code description
// b
a {color: #FF; } Demo. I think this is the correct behaviour, and what is currently implemented (if undocumented). However, I understand we probably want to discuss this more to weigh up the pros and cons. I think we have two options: The outcome of each option is the same:
I'm leaning towards the 2nd option as it's the behaviour we described in the changelog: "support for multi-line disable descriptions". If the heuristics prove too tricky to implement, we can fallback on the first option. Once the patch is released, we can then discuss the merits or not of changing the merging behaviour (in a major release). Sound good? @stof @hudochenkov This is quite the gnarly issue to unpick, and I appreciate your time so far unpicking together. Thanks! |
I agree on trying the 2nd option first, so that most regressions on existing cases are solved while still preserving support for multiline description. And the rules about how to use command comments in scss should be clearly documented ( Then, if you want to remove the heuristics in a future major version, I think this would require to trigger some deprecation warning for cases where the heuristics enters into action to prevent merging, so that users can discover these cases (could even be something fixed automatically by adding empty lines). |
Second option sound the best for me as well. @stof could you combine two of your comments (#4937 (comment) and #4937 (comment)) into one set of instructions how it should work. And gather all code examples mentioned here which we need to support. It would help person, who's going to fix this. Or you could send pull request with fixed behavior and added tests :) |
I'll label as ready to implement. If anyone has time to address this, please shout and label the issue as wip. cc-ing in @jathak as the author of the feature, in case they have time. |
I can work on this now. Just to confirm, the desired logic is that merging should only start when a comment starts with Edit: Plus |
I've wrote comment before you've added more info to your comment, @jathak :) I think the complete list of rules would be:
|
I'll try to release tomorrow. |
I'll hopefully have time on Saturday if you don't get around to releasing first. |
This is a follow-up of #4913 which fixed parts of the regression by avoiding to merge double-slash comments separated by empty lines.
There is still a regression when a disabling comment is in a double-shash comment which is not separated from preceding comments with a empty line, as the
stylelint-disable
is not on the at the beginning of the comment anymore after merging.Can be any rule as it is related to disabling comments
See https://github.com/twbs/bootstrap/blob/5c25ea647bdc463df5a322c8248460a78305a1f4/scss/bootstrap-grid.scss#L24-L27 for an example
e.g.
13.7.1
e.g. "CLI with
stylelint "**/*.css" --config myconfig.json
"Yes, it's related to SCSS double-slash comments
rules are properly disabled and re-enabled, as done in Stylelint 13.6
Some of these control comments are ignored
The text was updated successfully, but these errors were encountered: