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

Add function-calc-no-invalid rule #3833

Merged
merged 4 commits into from Dec 27, 2018
Merged

Conversation

ota-meshi
Copy link
Member

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

Closes #3820

Is there anything in the PR that needs further explanation?

This PR add function-calc-no-invalid rules.
A new rule that reports an invalid expression of the calc() function.


I created calc expression parser with reference to parser.jison of postcss-calc.
The main changes from parser.jison of postcss-calc are below.

  • Changed to case insensitive.
  • Changed to accept an unknown expression.
  • Changed to include location in node.
  • Added predefined units with stylelint. (e.g. vm)
  • Changed the calculation process of minus.
  • Changed to integrate length units into LengthValue.

@jeddy3
Copy link
Member

jeddy3 commented Dec 3, 2018

Oh wow, this is great!

My original concern was that this rule would not meet the following criteria for inclusion:

  • Has a clear and unambiguous finished state.

However, because this rule is using a spec compliant parser, I think it might meet this criteria.

Can anyone think of a reason not to include this rule in core?

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.

Very very good job, need test nested calc

@ota-meshi
Copy link
Member Author

@evilebottnawi Thank you for review!
I added test cases and fixed bugs.

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.

Great job!

I think we should union with postcss-calc and move parser as separate package or include this logic in postcss-value-parser, but don't have time on this right now.

@jeddy3 jeddy3 changed the title WIP: Add function-calc-no-invalid rule Add function-calc-no-invalid rule Dec 6, 2018
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.

I think we need more tests around scss, less and css-in-js (?) syntaxes (variables, nested function). Example: calc(10px + $var)? Thanks for good job again

@ota-meshi
Copy link
Member Author

I think we should union with postcss-calc and move parser as separate package or include this logic in postcss-value-parser, but don't have time on this right now.

True, let me know if I can help.

I think we need more tests around scss, less and css-in-js (?) syntaxes (variables, nested function). Example: calc(10px + $var)?

I'm not familiar with less and css-in-js, so I will gradually catch up and add test cases.

@alexander-akait
Copy link
Member

@ota-meshi let's add some tests with variables $var, @var and functions calc(10px + myFunc()) and we can merge

@ota-meshi
Copy link
Member Author

ota-meshi commented Dec 10, 2018

I added the test cases.
Please tell me if the test cases is not enough.

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.

LGTM

@evilebottnawi Any further requests, or does this look good to you now?

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.

Looks good 👍

@jeddy3 jeddy3 merged commit fad0cbc into master Dec 27, 2018
@jeddy3 jeddy3 deleted the add/function-calc-no-invalid branch December 27, 2018 11:21
@jeddy3
Copy link
Member

jeddy3 commented Dec 27, 2018

@ota-meshi Thanks for adding this epic new rule!

  • Added: function-calc-no-invalid rule (#3833).

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.

None yet

3 participants