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.

Sign up for GitHub

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

Jump to bottom

Fix false positives in nested property declarations unit-no-unknown #5500

Merged
merged 3 commits into from Sep 4, 2021
Merged

Fix false positives in nested property declarations unit-no-unknown #5500

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
323498d
Fix false positives in nested property declarations unit-no-unknown
sylwia-lask Aug 26, 2021
4691bed
use isStandardSyntaxDeclaration in unit-no-unknow util
sylwia-lask Sep 1, 2021
ec1deb5
CR fixes
sylwia-lask Sep 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 18 additions & 0 deletions lib/rules/unit-no-unknown/__tests__/index.js
View file
Open in desktop
Expand Up @@ -453,6 +453,18 @@ testRule({
code: '$breakpoints: ( small: /* comment */ 767px, medium: ( 1prop: 992px, 2prop: ( 1prop: 1200px ) ) );',
description: 'ignore map property name in nested maps',
},
{
code: '$foo: ( small: 400px, medium: 400px * 2, large: 4 * 400px )',
description: 'work with multiplication inside map statement',
},
{
code: '$foo: ( small: 400px, medium: 400px*2, large: 4*400px )',
description: 'work with multiplication inside map statement - no whitespaces',
},
{
code: '$foo: ( small: 400px, 1unit: 400px * 2, 3tx: 4 * 400px )',
description: 'ignore map property names when use multiplication inside map',
},
],

reject: [
Expand Down Expand Up @@ -516,6 +528,12 @@ testRule({
line: 1,
column: 37,
},
{
code: '$foo: ( small: 400px, medium: 400pix * 2, large: 4 * 400px )',
message: messages.rejected('pix'),
line: 1,
column: 31,
},
],
});

Expand Down
8 changes: 3 additions & 5 deletions lib/rules/unit-no-unknown/index.js
View file
Open in desktop
Expand Up @@ -22,10 +22,6 @@ const messages = ruleMessages(ruleName, {
rejected: (unit) => `Unexpected unknown unit "${unit}"`,
});

// The map property name (in map cleared from comments and spaces) always
// has index that being divided by 4 gives remainder equals 0
const mapPropertyNameIndexOffset = 4;
Copy link
Member

Choose a reason for hiding this comment

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

[question] This mapPropertyNameIndexOffset constant (and also isMap) was introduced with PR #4450. I'm not sure the context, but it seems better to me to improve isStandardSyntaxDeclaration as @jeddy3 suggested on #5434 (comment), instead of changing the current logic.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ybiquitous this PR doesn't change current logic. It just changes the assumption, that the mapPropertyNameIndexOffset is always 4 - because it isn't, when map value has "*" inside. I've changed it, that now we check that property ends with ":", which is much more reliable in my opinion.

If we'll use isStandardSyntaxDeclaration it will disable this rule for all scss maps. Which I guess we don't want to do, because now it works ok, instead of the edge case with "*" in value. And for sure we can't make this kind of logic in 119 line, since upper "if" is if (unit.toLowerCase() === 'x') (what with the other units?)

What do you think @jeddy3, am I thinking correctly?

Copy link
Member

Choose a reason for hiding this comment

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

@sylwia-lask Thanks for your pull request.

Let's remove the map functionality, including isMap, as it's inconsistent with how all the other built-in rules deal with non-standard syntax, i.e. by ignoring them using the isStandardSyntax* utils. Code for non-standard syntax only belongs in the isStandardSyntax* utils.

The built-in rules are for standard CSS constructs, and so this rule should ignore maps.

Someone can then write a scss/unit-no-unknown plugin for stylelint-scss that specifically checks for unknown units in Sass constructs like maps. The plugin can deal with the awkward edge cases that come from working with SCSS syntax using parsers designed for CSS). The plugin would be used in tandem with the built-in rule.

Ref: #5454 (review)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jeddy3 in test file for unit-no-unknown are lots of tests for less, sass and scss syntaxes. Should I remove it?

Copy link
Member

Choose a reason for hiding this comment

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

Let's just remove the reject tests that will fail when we ignore the constructs outright. The accept tests will remain valid as they'll now test that the constructs are ignored.

When you remove the map code and add the (amended) isStandardSyntaxDeclaration util, it should become clear which tests these are.


function rule(actual, options) {
return (root, result) => {
const validOptions = validateOptions(
Expand Down Expand Up @@ -66,7 +62,9 @@ function rule(actual, options) {

if (isMap(valueNode)) {
valueNode.nodes.forEach(({ sourceIndex }, index) => {
if (!(index % mapPropertyNameIndexOffset)) {
const nextNode = valueNode.nodes[index + 1];

if (nextNode && nextNode.value === ':') {
ignoredMapProperties.push(sourceIndex);
}
});
Expand Down