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

Fix block-no-empty false positives for reportNeedlessDisables #6381

Merged
merged 1 commit into from Oct 2, 2022

Conversation

ybiquitous
Copy link
Member

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

Closes #5388

Is there anything in the PR that needs further explanation?

As suggested in #5388 (comment), this PR extracts a new utility from assignDisabledRanges.js, and then it ignores a Stylelint command comment in the block-no-empty rule by using the new utility.

@changeset-bot
Copy link

changeset-bot bot commented Oct 1, 2022

🦋 Changeset detected

Latest commit: a30c5c3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
stylelint Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

return (
statement.nodes !== undefined && statement.nodes.length === 0 // has block
); // and is empty
return hasBlock(statement) && statement.nodes.length === 0;
Copy link
Member Author

Choose a reason for hiding this comment

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

[note] It should be more readable and less duplicated thanks to hasBlock:

module.exports = function hasBlock(statement) {
return statement.nodes !== undefined;
};

function extractStylelintCommand(comment) {
const [command] = comment.text.split(/\s/, 1);

assertString(command);
Copy link
Member Author

Choose a reason for hiding this comment

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

[note] Since ''.split(/\s/, 1) returns [''], this assertString should prevent command is undefined.

@ybiquitous ybiquitous force-pushed the fix-false-positives-for-reportNeedlessDisables branch from 9e44543 to a30c5c3 Compare October 1, 2022 13:48
@ybiquitous ybiquitous marked this pull request as ready for review October 1, 2022 13:52
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.

This is fantastic, thanks!

LGTM.

@ybiquitous ybiquitous merged commit fbfdb59 into main Oct 2, 2022
@ybiquitous ybiquitous deleted the fix-false-positives-for-reportNeedlessDisables branch October 2, 2022 22:22
@AlexSkrypnyk
Copy link

Looks like this change broke correct ignore rules that worked before:

  &__top__content-top1 {
    // stylelint-disable-line block-no-empty
  }

results in

 24:24  ✖  Unexpected empty block  block-no-empty
$ npm list stylelint
├─┬ stylelint-config-standard-scss@2.0.1
│ ├─┬ stylelint-config-recommended-scss@5.0.2
│ │ ├─┬ stylelint-scss@4.3.0
│ │ │ └── stylelint@14.14.0 deduped
│ │ └── stylelint@14.14.0 deduped
│ └── stylelint@14.14.0 deduped
├─┬ stylelint-config-standard@23.0.0
│ ├─┬ stylelint-config-recommended@6.0.0
│ │ └── stylelint@14.14.0 deduped
│ └── stylelint@14.14.0 deduped
└── stylelint@14.14.0

@AlexSkrypnyk
Copy link

Ok, this works

  &__middle__content-middle1 {
    // stylelint-disable-block block-no-empty
  }

I could not find any information in this PR or the release about changes to the disable rule for blocks: from stylelint-disable-line to stylelint-disable-block

@ybiquitous
Copy link
Member Author

@AlexSkrypnyk stylelint-disable-block is not a valid Stylelint command. So the code above is equal to the following code using a non-special comment:

&__middle__content-middle1 {
  // This block is not empty because of including this comment
}

You can see it on the demo.

It seems you need to fix the code as below:

&__top__content-top1 { // stylelint-disable-line block-no-empty
}

Or

// stylelint-disable-next-line block-no-empty
&__top__content-top1 {
}

Or

&__top__content-top1 {
  // You can put any command except for the Stylelint commands here.
}

wmfgerrit pushed a commit to wikimedia/mediawiki-extensions-TimedMediaHandler that referenced this pull request Nov 14, 2022
Add missing word-break to stylelint-disable, which makes it working as
expected, but newer stylelint requires a word break.
Side-effect from stylelint/stylelint#6381

Mistake in ea6a419

Change-Id: If5a8317d82e3e4bdcd6ebf48f9d2af2aea040eb7
wmfgerrit pushed a commit to wikimedia/mediawiki-extensions that referenced this pull request Nov 14, 2022
* Update TimedMediaHandler from branch 'master'
  to d64a165c4ee526e6858fd9f97f3d93b39ec3d367
  - build: Fix stylelint-disable
    
    Add missing word-break to stylelint-disable, which makes it working as
    expected, but newer stylelint requires a word break.
    Side-effect from stylelint/stylelint#6381
    
    Mistake in ea6a419
    
    Change-Id: If5a8317d82e3e4bdcd6ebf48f9d2af2aea040eb7
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.

Fix block-no-empty false positives for reportNeedlessDisables
3 participants