Ignore maps and lists in color-named #2182

Merged
merged 1 commit into from Dec 17, 2016

Projects

None yet

4 participants

@mmase
Contributor
mmase commented Dec 9, 2016

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

#2009

Is there anything in the PR that needs further explanation?

No, it's self explanatory.

lib/rules/color-named/index.js
@@ -49,7 +50,7 @@ const rule = function (expectation, options) {
const namedColors = Object.keys(namedColorData)
root.walkDecls(decl => {
- if (propertySets.acceptCustomIdents.has(decl.prop)) {
+ if (propertySets.acceptCustomIdents.has(decl.prop) || !isStandardSyntaxDeclaration(decl)) {
@evilebottnawi
evilebottnawi Dec 9, 2016 Member

use isStandardSyntaxDeclaration we disabled check on color in variables, root and scss nested properties (see https://github.com/stylelint/stylelint/blob/master/lib/utils/isStandardSyntaxDeclaration.js).
@stylelint/core what do you think guys? I vote for the exclusion of non-standard syntax.

@jeddy3
jeddy3 Dec 10, 2016 Member

@mmase Thank you for the PR.

use isStandardSyntaxDeclaration we disabled check on color in variables, root and scss nested properties

I might have made a mistake suggesting the use of isStandardSyntaxDeclaration to resolve this issue as it's a bit of a sledgehammer. In #2009 we agreed to ignore only maps and lists. This is because we made a design decision a while back to try to be as fine-grained as reasonably possible when ignoring non-standard constructs. This is particularly applicable to rules that check things within values e.g. color-*, number-*, unit-* etc.

@mmase As such we might want to slightly rethink how we go about this. Perhaps the following would work?:

When we check a postcss-value-parser's word, we should first check if it has a parent node and if that parent node is a function, and then check if that function is a standard one using isStandardSyntaxFunction. We'll want to do this inside the walk.

This should ignore maps and lists like this:

colors: (white: #fff);

While not hammering nested props and variables.

Unfortunately, this is tricky first-stylelint-PR as it deals with the way postcss and postcss-value-parser parse non-standard syntax and constructs. Would you be willing to explore implementing this more fine-grained solution?

@mmase
mmase Dec 10, 2016 Contributor

Makes sense! Will dig in and make some changes

@mmase
mmase Dec 12, 2016 Contributor

@jeddy3 so far pretty straight forward, but the maps return false for the isStandardSyntaxFunction as maps aren't given a value by postcss-value-parser. Thoughts?

@mmase
mmase Dec 12, 2016 Contributor

I pushed a commit up with what seems to produce expected results, though sans isStandardSyntaxFunction due to the above.

@jeddy3 jeddy3 changed the title from Proposal: Ignore color-named rule in non-standard syntax to Ignore maps and lists in color-named Dec 10, 2016
@jeddy3

@mmase Thanks! I think this work well.

@davidtheclark Any thoughts on this approach?

lib/rules/color-named/index.js
valueParser(decl.value).walk(node => {
const value = node.value,
type = node.type,
sourceIndex = node.sourceIndex
+ if (type === "function") {
@jeddy3
jeddy3 Dec 12, 2016 edited Member

Could we get a brief comment above this line stating what this is here for as it's not so obvious and I'm sure we'll come back to it in a couple of months time wondering what it does? :)

@davidtheclark
davidtheclark Dec 12, 2016 Contributor

Yes, we definitely need a clarifying comment here. Also, I think the variable name functionNodes is misleading. You're not actually putting nodes in it, right? Could you please try to name that array to match its contents?

@mmase
Contributor
mmase commented Dec 12, 2016

Made requested changes, let me know if you have any other thoughts on improvements!

lib/rules/color-named/index.js
+ }
+
+ // Return early if node is inside of a function
+ if (functionNodeIndexes.indexOf(sourceIndex) > -1) {
@davidtheclark
davidtheclark Dec 12, 2016 Contributor

Can we accomplish the same thing in a more standard way with blurFunctionArguments? Have you checked out what is happening in other rules that also ignore function arguments?

@mmase
mmase Dec 12, 2016 Contributor

Was just about to mention that - as it appears that this is essentially the same behavior as the ignore inside-function option. Which makes me ask, what is #2009 asking of that can't be achieved with that option?

@mmase
mmase Dec 12, 2016 Contributor

Additionally, it appears returning false inside of a function node is all that needs to be done.

@davidtheclark
Contributor

@mmase I think the relevant suggestion in #2009 is

Perhaps we should change the behaviour to ignore maps by default, and reserve "ignore": ["inside-function"] for just things like $color-footer: color(white) !default;?

from @jeddy3.

If we can't ignore maps without doing the same thing as "ignore": ["inside-function"], then we should just leave things as they are. If we can ignore them by some other means, we'd do that.

@mmase
Contributor
mmase commented Dec 12, 2016

Ah right. Apologies, missed that. Okay, will try a few things as this current implementation feels too harsh.

@jeddy3
Member
jeddy3 commented Dec 12, 2016 edited

Made requested changes, let me know if you have any other thoughts on improvements!

Thank you!

I think this issue has highlighted a few shortcomings in the tests of this rule as it's not immediately obvious from them if this rule is behaving as expected.

I think that the default behaviour should be:

  1. Check color representations inside all standard syntax functions (e.g. when using color-mod: a { color: color(black a(50%)))
  2. Ignore non-standard list and map syntax i.e. (..)

Then we provide an "ignore": ["inside-function"] option for ignoring color representations within standard syntax functions.

I think we're missing at least two tests in the "never" set that show that colours within standard syntax functions should be checked by default e.g. accept and reject tests for a { color: color(black a(50%)).

And another test in the "always-where-possible" reject set. (There is one for the accept set already.)

I think these new reject tests will likely fail with the current implementation in this PR.

If we can ignore them by some other means, we'd do that.

I thought we might be able to use postcss-value-parser to distinguish between standard syntax functions (i.e. those that have a name (e.g. color(..) or map-get(..)) and non-standard syntax ones (i.e. those that don't (..)). @mmase Do you think this might be possible or did your initial investigation turn up a blank?

Thanks for sticking with us on this. Dealing with non-standard syntax is usually deceptively tricky.

@mmase
Contributor
mmase commented Dec 15, 2016

@jeddy3 Sorry for the delay on this. Declaration values should let us see if they are in a named function or non-standard syntax function (no name). However, I'm wondering if we need to go a step further.

For example, in this case, we should ignore white and grey:

$colors: (
  white: #000,
  grey: #333
);

But, in this case, we should probably still warn:

$colors: (
  white: white,
  grey: grey
);

Do you agree?

@davidtheclark
Contributor

@mmase Parsing a Sass map is not within the domain of stylelint — rather, a SCSS-specific plugin should take care of that. stylelint itself should just ignore the construction, avoiding mistaken warnings.

@mmase
Contributor
mmase commented Dec 15, 2016

@davidtheclark fair enough, that makes sense 👍

@jeddy3
Member
jeddy3 commented Dec 15, 2016

Parsing a Sass map is not within the domain of stylelint — rather, a SCSS-specific plugin should take care of that. stylelint itself should just ignore the construction, avoiding mistaken warnings.

👍

@mmase If you're interested, David goes into a bit more background to this problem space here.

And for some context specific to this instance... dollar-variables are parsed as decls. Nested props are parsed as decls within rules. In both instances, the value part of the declaration is made available within decl.value. This isn't the case with lists and maps, where the decl.value contains the property-value pairs of the map as a single string.

Therefore, it would require additional code within stylelint to parse this non-standard construct.

Which is why the suggestion is to use isStandardSyntaxFunction to ignore list and maps without hammering vars and nested props.

@mmase
Contributor
mmase commented Dec 15, 2016 edited

@jeddy3 thanks for that. isStandardSyntaxFunction is great for ignoring non-standard list and map syntax i.e. (..). So, that's done and makes sense (I was previously using isStandardSyntaxDeclaration which is incorrect).

However, the remaining issue is ignoring instances of color-mods like color(red) or other function calls myFunction(red). Are we looking to only ignore color-mods?

@jeddy3
Member
jeddy3 commented Dec 15, 2016

So, that's done and makes sense (I was previously using isStandardSyntaxDeclaration which is incorrect).

👍

However, the remaining issue is ignoring instances of color-mods like color(red) or other function calls myFunction(red). Are we looking to only ignore color-mods?

We're ignoring neither by default. There is already the ignore: ["inside-function"] option that the user can use to opt into this behaviour. (This option will ignore values in standard syntax functions, so that includes functions included in the spec (like color()) and custom functions like myFunction(red).)

It sounds like what you've done so far is what we need. If you've also added those tests, then I think you can push up what you've got and we'll be able to see if it's behaving as expected :)

@mmase
Contributor
mmase commented Dec 15, 2016 edited

I suppose that could be covered by "ignore": "inside-function"?

@mmase
Contributor
mmase commented Dec 15, 2016

Ha, just saw you commented the same :) Okay, will write some tests and push up 👍

@mmase
Contributor
mmase commented Dec 15, 2016

@jeddy3 @davidtheclark made some changes. thanks for your patience with me on this one. I hope this is the solution you are looking for!

@@ -70,6 +71,10 @@ const rule = function (expectation, options) {
return false
}
+ if (!isStandardSyntaxFunction(node)) {
+ return false
@evilebottnawi
evilebottnawi Dec 15, 2016 Member

Better use just return

@mmase
mmase Dec 15, 2016 edited Contributor

This doesn't appear to work. I don't know a ton about postcss-value-parser, but returning undefined on only a value would work. However, since we are making an assertion on a node, you need to return false for it to skip the values within that node. Otherwise, I would group it with the isStandardSyntaxValue check below this.

@jeddy3
jeddy3 requested changes Dec 15, 2016 edited View changes

Looking good! I've a request for one last test at line 266...

{
    code: "a { color: color(#000 a(50%)) }",
    message: messages.expected("black", "#000"),
    line: 1,
    column: 12,
  } ],

To make sure that we're checking inside of functions for the "always-where-possible" option.

lib/rules/color-named/__tests__/index.js
@@ -56,6 +56,9 @@ testRule(rule, {
}, {
code: "a { font: 10px/14px Brown, sans-serif; }",
description: "ignore font family names that are colors",
+ }, {
+ code: "colors: (white: #fff, blue: #00f);",
@jeddy3
jeddy3 Dec 15, 2016 Member

Let's use $colors here to make it extra clear this is a non-standard.

@jeddy3
Member
jeddy3 commented Dec 15, 2016 edited

@mmase Ooops, code for my test should be a { color: color(#000 a(50%)) }... I accidently left out the property name.

@mmase
Contributor
mmase commented Dec 15, 2016 edited

@jeddy3 ah, yeah I did the same thing a few times as well. all set

@mmase mmase Ignore maps and lists in color-named
b67ced8
@jeddy3
jeddy3 approved these changes Dec 15, 2016 View changes

Excellent. LGTM :)

@mmase
Contributor
mmase commented Dec 17, 2016

thanks guys!

@davidtheclark davidtheclark merged commit 5c38334 into stylelint:master Dec 17, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@davidtheclark
Contributor

Changelog entry:

  • Fixed: color-named now ignores SCSS maps, so map property names can be color names (#2182)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment