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

Ignore calc() in length-zero-no-unit #4175

Merged
merged 12 commits into from Aug 11, 2019
2 changes: 1 addition & 1 deletion lib/rules/length-zero-no-unit/README.md
@@ -1,6 +1,6 @@
# length-zero-no-unit

Disallow units for zero lengths.
Disallow units for zero lengths. Ignores `calc` value in favor of `function-calc-no-invalid` rule.
Copy link
Member

Choose a reason for hiding this comment

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

Minor doc nitpick. Let's revert the change to this sentence and add the following below the example:

This rule ignores lengths within `calc` functions in favor of the [`function-calc-no-invalid`](../function-calc-no-invalid/README.md) rule.

This is to be consistent with other rule READMEs e.g.
https://github.com/stylelint/stylelint/tree/master/lib/rules/font-family-name-quotes#font-family-name-quotes

Otherwise, this PR LGTM.

Copy link
Member Author

Choose a reason for hiding this comment

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

updated


```css
a { top: 0px; }
Expand Down
23 changes: 20 additions & 3 deletions lib/rules/length-zero-no-unit/__tests__/index.js
Expand Up @@ -12,6 +12,22 @@ testRule(rule, {
code: "a { top: 0; }",
description: "unitless zero"
},
{
description: "ignore calc",
code: "a { padding: calc(0px +\n 0px); }"
},
{
description: "ignore calc. insensitive",
code: "a { padding: cAlc(0px + 0px); }"
},
{
description: "ignore calc. empty calc",
code: "a { padding: calc(); }"
},
Copy link
Member

Choose a reason for hiding this comment

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

Can we add test like padding: calc(1in + 0in * 2)) 0in, should have one error

Copy link
Member Author

Choose a reason for hiding this comment

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

awesome suggestion. I have improve it to padding: calc(1in + 0in * 2)) 0in calc(0px) 0px since it was needed to rework regexp, too

{
description: "ignore calc. several `calc`s",
code: "a { padding: calc(1in + 0in * 2)) 0 calc(0px) 0 }"
},
{
code: "a { padding: 0 /* 0px */; }",
description: "united zero in comment"
Expand Down Expand Up @@ -247,12 +263,13 @@ testRule(rule, {
column: 27
},
{
code: "a { padding: calc(1in + 0in * 2)); }",
fixed: "a { padding: calc(1in + 0 * 2)); }",
description: "ignore calc. has another zero units",
code: "a { padding: calc(1in + 0in * 2)) 0in calc(0px) 0px }",
fixed: "a { padding: calc(1in + 0in * 2)) 0 calc(0px) 0 }",

message: messages.rejected,
line: 1,
column: 26
column: 36
Copy link
Member

Choose a reason for hiding this comment

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

And other hardcore example: padding: calc(var(--foo, 0in) + 10px) 0in 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

ignore: ["custom-properties"] does not work with such example, is it a bug?

Copy link
Member

Choose a reason for hiding this comment

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

i think yes, we catch a bug

},
{
code: "@media (min-width: 0px) {}",
Expand Down
24 changes: 16 additions & 8 deletions lib/rules/length-zero-no-unit/index.js
Expand Up @@ -3,6 +3,7 @@
const _ = require("lodash");
const beforeBlockString = require("../../utils/beforeBlockString");
const blurComments = require("../../utils/blurComments");
const getMathFunctionsRanges = require("../../utils/getMathFunctionsRanges");
const hasBlock = require("../../utils/hasBlock");
const isCustomProperty = require("../../utils/isCustomProperty");
const keywordSets = require("../../reference/keywordSets");
Expand All @@ -28,7 +29,10 @@ const rule = function(actual, secondary, context) {
}

root.walkDecls(decl => {
check(blurComments(decl.toString()), decl);
const stringValue = blurComments(decl.toString());
const ignorableRanges = getMathFunctionsRanges(stringValue);

check(stringValue, decl, ignorableRanges);
});

root.walkAtRules(atRule => {
Expand All @@ -39,17 +43,21 @@ const rule = function(actual, secondary, context) {
check(source, atRule);
});

function check(value, node) {
const ignorableIndexes = new Set();
const fixPositions = [];

function check(value, node, ignorableRanges = []) {
if (
optionsMatches(secondary, "ignore", "custom-properties") &&
isCustomProperty(value)
) {
return;
}

const ignorableIndexes = new Array(value.length).fill(false);
const fixPositions = [];

ignorableRanges.forEach(([start, end]) => {
for (let i = start; i < end; i++) ignorableIndexes[i] = true;
});

styleSearch({ source: value, target: "0" }, match => {
const index = match.startIndex;

Expand All @@ -66,7 +74,7 @@ const rule = function(actual, secondary, context) {
//
// This check prevents that from happening: we build and check against a
// Set containing all the indexes that are part of a value already validated.
if (ignorableIndexes.has(index)) {
if (ignorableIndexes[index]) {
return;
}

Expand Down Expand Up @@ -110,8 +118,8 @@ const rule = function(actual, secondary, context) {

// Add the indexes to ignorableIndexes so the same value will not
// be checked multiple times.
_.range(valueWithZeroStart, valueWithZeroEnd).forEach(i =>
ignorableIndexes.add(i)
_.range(valueWithZeroStart, valueWithZeroEnd).forEach(
i => (ignorableIndexes[i] = true)
);

// Only pay attention if the value parses to 0
Expand Down
18 changes: 18 additions & 0 deletions lib/utils/getMathFunctionsRanges.js
@@ -0,0 +1,18 @@
"use strict";

const MATH_FUNCTIONS = ["calc"];
const MATH_FUNCTIONS_REGEXP = new RegExp(
`(?:${MATH_FUNCTIONS.join("|")})` + "\\([^()]*(?:\\(.*\\))*[^()]*\\)",
"gi"
);
Copy link
Member

Choose a reason for hiding this comment

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

Please don't use regexp for this cases, we have postcss-value-parser, regexp is slow and unsafe

Copy link
Member Author

Choose a reason for hiding this comment

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

👌


module.exports = function getMathFunctionsRanges(valueString) {
const result = [];
let current;

while ((current = MATH_FUNCTIONS_REGEXP.exec(valueString)) !== null) {
result.push([current.index, current.index + current[0].length]);
}

return result;
};