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 positives for parametric mixins in selector-class-pattern #5378

Merged
merged 6 commits into from Jul 26, 2021

Conversation

immitsu
Copy link
Contributor

@immitsu immitsu commented Jun 18, 2021

Fix #5258

Noticed that with the added if-block there is no necessity in checker on lines 59-61. Probably, should be removed?
Furthermore, if to delete checkers on lines 21-44 in isStandardSyntaxRule.js all tests will be passed. Is it ok?

@jeddy3 jeddy3 changed the title Fix processing of less parametric mixins Fix false positives for parametric mixins in selector-class-pattern Jul 13, 2021
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.

Thanks for the pull request, and apologies for just getting around to reviewing it.

Can you please set to merge into the v14 branch.

Noticed that with the added if-block there is no necessity in checker on lines 59-61. Probably, should be removed? Furthermore, if to delete checkers on lines 21-44 in isStandardSyntaxRule.js all tests will be passed. Is it ok?

If the new conditional makes the previous ones redundant and the tests pass, then yes go ahead a remove them.

@immitsu immitsu force-pushed the fix-less-parametric-mixins branch from 158e277 to 15cca41 Compare July 18, 2021 19:56
@immitsu immitsu changed the base branch from master to v14 July 18, 2021 19:57
@immitsu immitsu marked this pull request as ready for review July 18, 2021 20:05
@immitsu immitsu requested a review from jeddy3 July 18, 2021 20:08
@ybiquitous
Copy link
Member

Hi @immitsu, thank you for creating this PR!

When I’m looking into isStandardSyntaxRule.test.js in detail, I find some test cases that seem to be inaccurate.

For example, the following test case is close to this case added by this PR:

it('called Less class parametric mixin', () => {
expect(isStandardSyntaxRule(lessNode('.mixin-name(@var);'))).toBeFalsy();
});

While .mixin-name(@var); is an at-rule node, .mixin-name(@var) {} is a rule node. Thus, I think the test case should be:

-expect(isStandardSyntaxRule(lessNode('.mixin-name(@var);'))).toBeFalsy();
+expect(isStandardSyntaxRule(lessNode('.mixin-name(@var) {}'))).toBeFalsy();

So, I think we should fix the test and refactor the tested code, and then fix this false positive. What do you think?

@immitsu
Copy link
Contributor Author

immitsu commented Jul 24, 2021

Hi @ybiquitous!

I think you're right, have fixed/deleted inaccurate test cases.
Thanks!

Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

@immitsu Thank you for addressing my reviews! I've left a few suggestions, so I would be glad if considering them. 😃

lib/utils/isStandardSyntaxRule.js Show resolved Hide resolved
lib/utils/isStandardSyntaxRule.js Outdated Show resolved Hide resolved
Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

@immitsu Thank you for your work! LGTM 👍🏼

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, thanks!

@jeddy3 jeddy3 merged commit 56e2453 into stylelint:v14 Jul 26, 2021
@jeddy3
Copy link
Member

jeddy3 commented Jul 26, 2021

v14 changelog entry:

  • Fixed: selector-class-pattern false positives for Less parametric mixins (#5378).

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 false positives for parametric mixins in selector-class-pattern
3 participants