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 support for multi-line single-line disable descriptions #4886

Closed
nex3 opened this issue Jul 30, 2020 · 5 comments · Fixed by #4895
Closed

Add support for multi-line single-line disable descriptions #4886

nex3 opened this issue Jul 30, 2020 · 5 comments · Fixed by #4895
Labels
status: wip is being worked on by someone syntax: scss relates to SCSS and SCSS-like syntax type: bug a problem with a feature or rule

Comments

@nex3
Copy link
Contributor

nex3 commented Jul 30, 2020

Clearly describe the bug

With CSS-style multiline comments, it's possible to write disable descriptions (#4848) that span multiple lines:

/* stylelint-disable declaration-no-important --
 * some long-winded description */

Unfortunately, this doesn't work with Sass-style single-line comments (when parsed as the SCSS syntax of course):

// stylelint-disable declaration-no-important --
// some long-winded description

Which rule, if any, is the bug related to?

N/A

What code is needed to reproduce the bug?

a {
  // stylelint-disable color-no-invalid-hex --
  // foo
  color: #FF;
}

What stylelint configuration is needed to reproduce the bug?

{
	"rules": {
		"color-no-invalid-hex": true
	}
}

Which version of stylelint are you using?

Tested with 30bacfc

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

CLI with stylelint test.css --config scripts/visual-config.json

Does the bug relate to non-standard syntax (e.g. SCSS, Less etc.)?

Yes, it's related to SCSS comments.

What did you expect to happen?

I expected the description to be ignored and the lint to be disabled.

What actually happened (e.g. what warnings or errors did you get)?

The lint was not disabled and still produced a warning

test.scss
 4:10  ✖  Unexpected invalid hex color "#FF"   color-no-invalid-hex
@hudochenkov
Copy link
Member

This is probably due CSS comment being a single comment entity. While in Sass-style it's two separate comments.

@nex3
Copy link
Contributor Author

nex3 commented Jul 30, 2020

It looks like you're right... if I add any text after the -- it parses correctly, which suggests that it's trying to parse stylelint-disable color-no-invalid-hex -- as a single entity and failing because it's looking for something like -- .* at the end.

As a user, though, I'd expect stylelint to treat multiple adjacent inline comments as a single unified comment (which is incidentally how they're exposed in Sass's own AST). For example, I'd also expect the following to disable the lint:

a {
  // stylelint-disable-next-line color-no-invalid-hex --
  // foo
  color: #FF;
}

Without support for this, #4868 would be of limited use in Sass files, since descriptions would have to be very short or risk overflowing line-length limits and producing readability problems.

@jeddy3
Copy link
Member

jeddy3 commented Aug 9, 2020

(which is incidentally how they're exposed in Sass's own AST)

That's very interesting. It would be great if postcss-scss matched this behaviour so that both the following are single comment nodes:

/* stylelint-disable declaration-no-important --
 * some long-winded description */
// stylelint-disable declaration-no-important --
// some long-winded description

@nex3 Can you raise this issue upstream about this? If it can't be resolved upstream, we can work around it in stylelint.

@jeddy3 jeddy3 changed the title Multiline disable descriptions don't work with Sass-style comments Add support for multi-line single-line disable descriptions Aug 9, 2020
@jeddy3 jeddy3 added syntax: scss relates to SCSS and SCSS-like syntax type: bug a problem with a feature or rule labels Aug 9, 2020
@nex3
Copy link
Contributor Author

nex3 commented Aug 10, 2020

Done: postcss/postcss-scss#109

In the meantime, @jathak is working on a pull request to work around this representation issue in stylelint.

@jeddy3
Copy link
Member

jeddy3 commented Aug 11, 2020

@jathak is working on a pull request to work around this representation issue in stylelint.

Good stuff. It looks like the change will be made upstream, but we can work around it in stylelint in the interim.

@jeddy3 jeddy3 added the status: wip is being worked on by someone label Aug 11, 2020
jathak added a commit that referenced this issue Aug 12, 2020
jathak added a commit that referenced this issue Aug 17, 2020
kevindew added a commit to kevindew/govuk-frontend that referenced this issue Sep 2, 2020
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 added a commit to kevindew/govuk-frontend that referenced this issue Sep 14, 2020
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: wip is being worked on by someone syntax: scss relates to SCSS and SCSS-like syntax type: bug a problem with a feature or rule
Development

Successfully merging a pull request may close this issue.

3 participants