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
3 changes: 3 additions & 0 deletions lib/reference/mathFunctions.js
@@ -0,0 +1,3 @@
"use strict";

module.exports = ["calc"];
2 changes: 2 additions & 0 deletions lib/rules/length-zero-no-unit/README.md
Expand Up @@ -10,6 +10,8 @@ a { top: 0px; }

*Lengths* refer to distance measurements. A length is a *dimension*, which is a *number* immediately followed by a *unit identifier*. However, for zero lengths the unit identifier is optional. The length units are: `em`, `ex`, `ch`, `vw`, `vh`, `cm`, `mm`, `in`, `pt`, `pc`, `px`, `rem`, `vmin`, and `vmax`.

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

The `--fix` option on the [command line](../../../docs/user-guide/cli.md#autofixing-errors) can automatically fix all of the problems reported by this rule.

## Options
Expand Down
36 changes: 33 additions & 3 deletions lib/rules/length-zero-no-unit/__tests__/index.js
Expand Up @@ -12,6 +12,26 @@ 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 }"
},
{
description: "ignore calc, but not inner functions",
code: "padding: calc(var(--foo, 0) + 10px) 0"
},
{
code: "a { padding: 0 /* 0px */; }",
description: "united zero in comment"
Expand Down Expand Up @@ -247,12 +267,22 @@ testRule(rule, {
column: 27
},
{
code: "a { padding: calc(1in + 0in * 2)); }",
fixed: "a { padding: calc(1in + 0 * 2)); }",
description: "ignore calc, but not inner functions",
code: "padding: calc(var(--foo, 0in) + 10px) 0px",
fixed: "padding: calc(var(--foo, 0) + 10px) 0",

message: messages.rejected,
line: 1,
column: 27
},
{
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
33 changes: 25 additions & 8 deletions lib/rules/length-zero-no-unit/index.js
Expand Up @@ -5,6 +5,7 @@ const beforeBlockString = require("../../utils/beforeBlockString");
const blurComments = require("../../utils/blurComments");
const hasBlock = require("../../utils/hasBlock");
const isCustomProperty = require("../../utils/isCustomProperty");
const isMathFunction = require("../../utils/isMathFunction");
const keywordSets = require("../../reference/keywordSets");
const optionsMatches = require("../../utils/optionsMatches");
const report = require("../../utils/report");
Expand All @@ -28,7 +29,24 @@ const rule = function(actual, secondary, context) {
}

root.walkDecls(decl => {
check(blurComments(decl.toString()), decl);
const stringValue = blurComments(decl.toString());
const ignorableIndexes = new Array(stringValue.length).fill(false);
const parsedValue = valueParser(stringValue);

parsedValue.walk(node => {
if (node.type !== "function") {
return;
}

const stringValue = valueParser.stringify(node);
const ignoreFlag = isMathFunction(node);

for (let i = 0; i < stringValue.length; i++) {
ignorableIndexes[node.sourceIndex + i] = ignoreFlag;
}
});

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

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

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

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

const fixPositions = [];

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

Expand All @@ -66,7 +83,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 +127,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/__tests__/isMathFunction.test.js
@@ -0,0 +1,18 @@
"use strict";

const isMathFunction = require("../isMathFunction");
const valueParser = require("postcss-value-parser");

describe("isMathFunction", () => {
it("matches calc", () => {
expect(isMathFunction(valueParser("calc(10% + 10px)").nodes[0])).toBe(true);
});

it("matches calc. insensitive", () => {
expect(isMathFunction(valueParser("cALc(10% + 10px)").nodes[0])).toBe(true);
});

it("does not match var", () => {
expect(isMathFunction(valueParser("var(--foo, 1px)").nodes[0])).toBe(false);
});
});
17 changes: 17 additions & 0 deletions lib/utils/isMathFunction.js
@@ -0,0 +1,17 @@
/* @flow */
"use strict";

const MATH_FUNCTIONS = require("../reference/mathFunctions");

/**
* Check whether a node is math function
*
* @param {Node} postcss-value-parser node
* @return {boolean} If `true`, the node is math function
*/
module.exports = function isMathFunction(node /*: Object */) /*: boolean*/ {
return (
node.type === "function" &&
MATH_FUNCTIONS.includes(node.value.toLowerCase())
);
};