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 false negatives for CSS-In-JS in color-no-invalid-hex #3945 #3957

Merged
merged 2 commits into from Feb 21, 2019

Conversation

3 participants
@fogost
Copy link
Contributor

commented Feb 19, 2019

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

Closes #3945

Is there anything in the PR that needs further explanation?

This solves the problem, but I'm not sure about tests.

@jeddy3
Copy link
Member

left a comment

@fogost Thanks for making start on this and for your first-time contribution.

I've made some requests.

Your best bet is to add a failing test for css-in-js, and then migrate to postcss-value-parser to see if that fixes it. Here's an example of an css-in-js test.

@@ -28,9 +28,6 @@ testRule(rule, {
{
code: "a { padding: 000; }"
},
{

This comment has been minimized.

Copy link
@jeddy3

jeddy3 Feb 19, 2019

Member

This test needs to stay as we need to prove that this rule doesn't check the contents of strings.

return;
}
styleSearch(
{ source: declString, target: "#", strings: "check" },

This comment has been minimized.

Copy link
@jeddy3

jeddy3 Feb 19, 2019

Member

I suspect the way to fix this is to migrate the rule away from styleSearch and to postcss-value-parser. There wasn't a value parser around when this rule was created, but now we have one and it is proving a more robust way of dealing with the values of declarations.

There an example of the value parser in use in the color-named rule.

e.g.

root.walkDecls(decl => {
  valueParser(decl.value).walk(({value, type, sourceIndex) => {
    if (type !== "word") return;
    const hexMatch = /^#[0-9A-Za-z]+/.exec(value);
    ..
  }
}
fogost
@fogost

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

I changed styleSearch to postcss-value-parser and added css-in-js tests

@jeddy3

jeddy3 approved these changes Feb 21, 2019

Copy link
Member

left a comment

@fogost Thanks very much for making the changes. It's great to see this rule (one of the first to be added to stylelint) get some love and be updated to use the value parser.

The changes look good to me.

There are a few pretty-printing changes to unrelated files in this PR. I suspect this is because they somehow slipped through the net in the past, and you (correctly) ran npm run prettier:fix before pushing your commit. I'm happy for these changes to be bundled in this PR for the sake of keeping things simple.

I'll create an issue to run npm run prettier:check as part of our CI tests.

This was referenced Feb 21, 2019

ntwb added a commit that referenced this pull request Feb 21, 2019

Run prettier:check as pretest (#3962)
<!---
Please read the following. Pull requests that do not adhere to these guidelines will be closed.

Each pull request must, with the exception of minor documentation fixes, be associated with an open issue. If a corresponding issue does not exist please stop. Instead, create an issue so we can discuss the change first.

If there is an associated open issue, then the next step is to make sure you've read the relevant developer guide:

- Creating a new rule: https://github.com/stylelint/stylelint/blob/master/docs/developer-guide/rules.md#creating-a-new-rule

- Adding an option to an existing rule: https://github.com/stylelint/stylelint/blob/master/docs/developer-guide/rules.md#adding-an-option-to-an-existing-rule

- Fixing a bug in an existing rule: https://github.com/stylelint/stylelint/blob/master/docs/developer-guide/rules.md#fixing-a-bug-in-an-existing-rule

Once you've done that, then please continue by answering these two questions:  -->

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

Ref: #3957 (review)

> Is there anything in the PR that needs further explanation?

We should use the CI to make sure everything is pretty-printed.

This will fail until #3957 is in and rebased.
@hudochenkov
Copy link
Member

left a comment

Looks good to me!

@jeddy3 jeddy3 merged commit ee266b6 into stylelint:master Feb 21, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.001%) to 96.362%
Details
@jeddy3

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

Changelog:

  • Fixed: color-no-invalid-hex false negatives for CSS-in-JS (#3957).

@fogost Thanks again for your first contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.