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 nested property scss syntax false positive #3283

Merged
merged 6 commits into from Apr 30, 2018

Conversation

4 participants
@brneto
Contributor

brneto commented Apr 28, 2018

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

issues #3230 #3130 #2642

Is there anything in the PR that needs further explanation?

The problem was in the no-descending-specificity rule. When it was calling PostCSS API method root.walkRules it was treating padding: as a selector and passing it to parseSelector directly, which was throwing the error ✖ Cannot parse selector parseError.

@evilebottnawi

Can we add tests?

@brneto

This comment has been minimized.

Contributor

brneto commented Apr 28, 2018

Yes, feel free to do that!

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Apr 28, 2018

@brneto i can't do this in your fork/branch 😄

@brneto

This comment has been minimized.

Contributor

brneto commented Apr 28, 2018

Give me a little help on that, please! Where should be the right folder to put this test lib/utils/test or lib/test? Should I create a new file or used an existent? If I need to use an existent which would be that?

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Apr 28, 2018

@brneto Create new file lib/utils/__tests__/isNestedProperty.test.js. I think you can use isCustomPropertySet.test.js as a reference for how your test will look like.

@brneto

This comment has been minimized.

Contributor

brneto commented Apr 28, 2018

I've just created the test for isNestedProperty, but how about the test for no-descending-specificity rule? Did you use to create a test for each rule? If so, in which file is it?

@brneto

This comment has been minimized.

Contributor

brneto commented Apr 28, 2018

I've created the test for no-descending-specificity rule. There are any other tests I need to create?

@evilebottnawi

Be great add tests for pair affected rules

});
});
it("rejects nested property", () => {

This comment has been minimized.

@evilebottnawi

evilebottnawi Apr 28, 2018

Member

rejects not nested property

This comment has been minimized.

@brneto

brneto Apr 28, 2018

Contributor

But I have done that, no?! If foo: {}, is a nested property. If foo: red, isn't a nested property!

Pardon me, if I don't understand your request, I'm a little new in contributing to projects!

This comment has been minimized.

@brneto

brneto Apr 28, 2018

Contributor

I get it no! Consider done!

This comment has been minimized.

@evilebottnawi

evilebottnawi Apr 28, 2018

Member

test inside it describe what happens, i.e. should be rejects not nested properties

This comment has been minimized.

@brneto

brneto Apr 28, 2018

Contributor

@evilebottnawi Sorry, my misunderstanding! On a second read, I got what you were meaning!

Thanks very much for all the help!!

@evilebottnawi

Good work. Let's wait second eyes for review

@hudochenkov

Great job! Congratulations on your first contribution to stylelint!

@brneto

This comment has been minimized.

Contributor

brneto commented Apr 28, 2018

Very thanks you all! I happy to have my first contribution done and accepted!!! 😃

@brneto

This comment has been minimized.

Contributor

brneto commented Apr 28, 2018

When are you planning delivery the version with this fix?

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Apr 30, 2018

@brneto just add Fixed: description to CHANGELOG into HEAD section and we can merge 👍

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Apr 30, 2018

@evilebottnawi users shouldn't update changelog. Maintainers do. See the third point https://stylelint.io/developer-guide/pull-requests/.

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented Apr 30, 2018

@hudochenkov oh, sorry, you are right

@hudochenkov hudochenkov merged commit 34cd970 into stylelint:master Apr 30, 2018

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 increased (+0.02%) to 95.983%
Details
@hudochenkov

This comment has been minimized.

Member

hudochenkov commented Apr 30, 2018

  • Fixed: no-descending-specificity false positives for Sass nested properties (#3283).
@jeddy3

This comment has been minimized.

Member

jeddy3 commented May 1, 2018

Sorry, it's been a busy couple of weeks and only just catching up on stylelint issues.

Unless I've missed something, isn't a { b: {} } non-standard syntax and already covered by isStandardSyntaxRule util?:

// Ignore Scss nested properties
if (selector.slice(-1) === ":") {
return false;
}

If so, can we create a follow-up issue to use isStandardSyntaxRule in no-descending-specificity and remove this new util? There are no other is* utils for identifying non-standard constructs. There are only is* utils for standard constructs like isCustomProperty and isCustomElement. The non-standard syntax hueristics are restricted to the isStandardSyntax* utils to keep things manageable.

There are a lot of moving parts in stylelint, especially when it comes to the utils, and I don't think anyone one person knows the workings of the entire system. So, I'm not surprised the isStandardSyntaxRule util might have been missed in this instance :)

@brneto

This comment has been minimized.

Contributor

brneto commented May 1, 2018

@jeddy3
I think you right! I don't know very well all the utils, that why I did that!

I'm going to change what you said and send a new PR, ok?

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented May 1, 2018

So, I'm not surprised the isStandardSyntaxRule util might have been missed in this instance :)

😌

I'm going to change what you said and send a new PR, ok?

@brneto It would be great!

@brneto

This comment has been minimized.

Contributor

brneto commented May 5, 2018

When are the team planning to release a new version of this linter?

bors bot added a commit to mozilla/delivery-console that referenced this pull request May 16, 2018

Merge #161
161: Update dependency stylelint to v9.2.1 r=rehandalal a=renovate[bot]

This Pull Request updates dependency [stylelint](https://github.com/stylelint/stylelint) from `v9.2.0` to `v9.2.1`



<details>
<summary>Release Notes</summary>

### [`v9.2.1`](https://github.com/stylelint/stylelint/blob/master/CHANGELOG.md#&#8203;921)
[Compare Source](stylelint/stylelint@9.2.0...0a7c7ed)
-   Fixed: `cache` option hiding CssSyntaxError outputs ([#&#8203;3258](`stylelint/stylelint#3258)).
-   Fixed: regression with processors (e.g. styled-components) ([#&#8203;3261](`stylelint/stylelint#3261)).
-   Fixed: `no-descending-specificity` false positives for Sass nested properties ([#&#8203;3283](`stylelint/stylelint#3283)).
-   Fixed: `selector-pseudo-class-no-unknown` false positives proprietary webkit pseudo classes when applied to a simple selector ([#&#8203;3271](`stylelint/stylelint#3271)).

---

</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).

Co-authored-by: Renovate Bot <bot@renovateapp.com>
@jeddy3

This comment has been minimized.

Member

jeddy3 commented May 16, 2018

When are the team planning to release a new version of this linter?

9.2.1 was just released with this fix in it. Sorry for the delay. There was tricky regression that took some time to work through.

@brneto

This comment has been minimized.

Contributor

brneto commented May 16, 2018

Thanks very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment