Fix false positives in nested property declarations unit-no-unknown #5500
Conversation
There was a problem hiding this comment.
@sylwia-lask Thanks for the contribution! This change sounds good enough, but I've left a question.
| @@ -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; | |||
There was a problem hiding this comment.
[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?
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
@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)
There was a problem hiding this comment.
@jeddy3 in test file for unit-no-unknown are lots of tests for less, sass and scss syntaxes. Should I remove it?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@sylwia-lask Thanks!
It's looking good. I've requested two changes in addition to @ybiquitous request.
lib/rules/unit-no-unknown/index.js
Outdated
| @@ -47,11 +43,14 @@ function rule(actual, options) { | |||
| } | |||
|
|
|||
| function check(node, value, getIndex) { | |||
| if (node.type === 'decl' && !isStandardSyntaxDeclaration(node)) { | |||
There was a problem hiding this comment.
Let's move this into the walkDecls at the bottom of the file, e.g.
root.walkAtRules((atRule) => {
if (!/^media$/i.test(atRule.name) && !atRule.variable) {
return;
}
check(atRule, atRule.params, atRuleParamIndex);
});
root.walkDecls((decl) => {
if (!isStandardSyntaxDeclaration(decl) {
return;
}
check(decl, decl.value, declarationValueIndex));
}So that it's grouped with the at-rule conditional.
Note: we should replace, in a separate refactor pull request, the && !atRule.variable with the isStandardSyntaxAtRule util.
There was a problem hiding this comment.
Thanks for review :) I can change the && !atRule.variable in separate pull request, do I need to create an issue for that?
There was a problem hiding this comment.
I can change the
&& !atRule.variablein separate pull request, do I need to create an issue for that?
Fantastic! No need to create a new issue. Just link to this thread in the pull request note.
| @@ -36,6 +37,11 @@ module.exports = function (decl) { | |||
| return false; | |||
| } | |||
|
|
|||
| // SCSS map and list declarations | |||
| if (value[0] === '(' && value[value.length - 1] === ')') { | |||
There was a problem hiding this comment.
Let's use string functions for this, which we've moved to in the other isStandardSyntax* utils:
| if (value[0] === '(' && value[value.length - 1] === ')') { | |
| if (value.startsWith('(') && value.endsWith(')')) { |
|
|
Closes #5434
The problem was the assumption that property in map always has index divided by 4. It isn't true when there is multiplier ("*") in the value. I changed the condition and now it works fine.