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

Conversation

vankop
Copy link
Member

@vankop vankop commented Jul 26, 2019

Which issue, if any, is this issue related to?

Closes #3037

Is there anything in the PR that needs further explanation?

nothing

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

One note, anyway thanks for helping

check(blurComments(decl.toString()), decl);
const stringValue = blurComments(decl.toString());

if (!/calc\((.|\s)*\)/i.test(stringValue)) {
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 move this to util and name file isMathFunction and put logic in utils (in next css spec we will have more math functions, not only calc)

Copy link
Member Author

Choose a reason for hiding this comment

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

done

{
description: "ignore calc. empty calc",
code: "a { padding: calc(); }"
},
Copy link
Member

@alexander-akait alexander-akait Jul 27, 2019

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

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Let's add more test, looks better

@vankop vankop requested a review from alexander-akait July 27, 2019 13:28

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

@alexander-akait alexander-akait Jul 27, 2019

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

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.

👌

@vankop vankop requested a review from alexander-akait July 27, 2019 20:42
Copy link
Member

@hudochenkov hudochenkov left a comment

Choose a reason for hiding this comment

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

Nice!

@@ -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


Disallow units for zero lengths.
Disallow units for zero lengths. 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.
Copy link
Member

Choose a reason for hiding this comment

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

Can you please move the "This rule ignores" sentence to below the example code block as we use this documentation pattern in lots of other places (like so).

Copy link
Member Author

Choose a reason for hiding this comment

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

Hope now is fine

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.

Great, thanks for making the doc change. LGTM.

module.exports = function isMathFunction(node /*: Object */) /*: boolean*/ {
return (
node.type === "function" &&
MATH_FUNCTIONS.indexOf(node.value.toLowerCase()) > -1
Copy link
Member

Choose a reason for hiding this comment

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

We can use this MATH_FUNCTIONS.includes(node.value.toLowerCase()), indexOf is very old, we can use more readable code 😄

And move MATH_FUNCTIONS to https://github.com/stylelint/stylelint/tree/master/lib/reference for better maintenance

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@hudochenkov hudochenkov changed the title length-zero-no-unit ignores calc Ignore calc() in length-zero-no-unit Aug 11, 2019
@hudochenkov hudochenkov merged commit 830b46b into stylelint:master Aug 11, 2019
@hudochenkov
Copy link
Member

  • Fixed: length-zero-no-unit false positives for inside calc function (#4175).

@vankop vankop deleted the fix-length-zero-no-unit branch August 30, 2019 19:35
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 inside calc function in to length-zero-no-unit
4 participants