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

Fixed: ignore less guards in selector-descendant-combinator-no-non-space. #2557

Merged
merged 1 commit into from
Jun 5, 2017

Conversation

alexander-akait
Copy link
Member

@alexander-akait alexander-akait commented May 7, 2017

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

#2110

Is there anything in the PR that needs further explanation?

No, it's self explanatory.

@alexander-akait alexander-akait changed the title Fixed: ignore less guards in `selector-descendant-combinator-no-non-s… Fixed: ignore less guards in selector-descendant-combinator-no-non-space. May 7, 2017
@jeddy3
Copy link
Member

jeddy3 commented May 8, 2017

Does this heuristic introduce a false negative for this valid CSS?

when     a { }

Where when is an unknown, yet valid, type selector.

@alexander-akait
Copy link
Member Author

@jeddy3 hm, right, let's add more complexly check

@alexander-akait
Copy link
Member Author

/cc @jeddy3

@jeddy3
Copy link
Member

jeddy3 commented May 13, 2017

Thanks. Can we add get truthy tests in the util for a when {} and when a {} please?

@alexander-akait
Copy link
Member Author

/cc @jeddy3 done

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.

LGTM

@@ -37,6 +37,11 @@ module.exports = function (rule/*: Object*/)/*: boolean*/ {
return false
}

// Less guards
if ((/when.*?\(.*?\)/).test(selector)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

only a whitespace can separate when and (, right? when\s+\(.*?\)

Copy link
Member Author

Choose a reason for hiding this comment

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

@davidtheclark no, allow use when not (@variable = true)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the regexp should account for this limited scenario, instead of allowing for any arbitrary characters between when and (.

Copy link
Member Author

Choose a reason for hiding this comment

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

@davidtheclark in theory allow when not not (@variable = true) (does not make sense)

Copy link
Contributor

Choose a reason for hiding this comment

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

@evilebottnawi: You can make the regexp as complicated as you think necessary — but I think it's not great to make it so simple that it might raise false positives.

Copy link
Member Author

Choose a reason for hiding this comment

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

@davidtheclark What solution is better to use here?

Copy link
Contributor

Choose a reason for hiding this comment

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

How about /when\s+(not\s+)*\(/? I think it will just take writing tests for what should and should not work and fiddling with the regexp until that's what happens.

@alexander-akait
Copy link
Member Author

/cc @davidtheclark

Copy link
Contributor

@davidtheclark davidtheclark left a comment

Choose a reason for hiding this comment

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

👍

@jeddy3 jeddy3 merged commit 5796ac4 into master Jun 5, 2017
@jeddy3 jeddy3 deleted the issue-2110 branch June 5, 2017 09:36
@jeddy3
Copy link
Member

jeddy3 commented Jun 5, 2017

Changelog:

  • Fixed: selector-descendant-combinator-no-non-space now ignores Less guards (#2557).

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.

None yet

3 participants