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.

Sign up for GitHub

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

Jump to bottom

Should not throw an exception when unknow function exist in calc #40

Closed
wants to merge 1 commit into from
Closed

Should not throw an exception when unknow function exist in calc #40

wants to merge 1 commit into from

Conversation

Semigradsky
Copy link
Collaborator

Fix #34

@Semigradsky Semigradsky requested a review from ben-eb November 9, 2017 09:21
@Semigradsky
Copy link
Collaborator Author

@andyjansson Hi, could you check changes?

constant/env is not approved yet
w3c/csswg-drafts#1693

andyjansson
andyjansson reviewed Nov 9, 2017
View reviewed changes
parser.jison
@@ -39,6 +39,7 @@

(calc) return 'NESTED_CALC';
(var\([^\)]*\)) return 'CSS_VAR';
([a-z]+\([a-z0-9-]+\)) return 'UNKNOW_FUNCTION'
Copy link
Contributor

Choose a reason for hiding this comment

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

The regex doesn't seem very resilient to me. Furthermore it might be worth normalising this and the CSS variable stuff to a single thing. \cc @MoOx what do you think?

andyjansson
andyjansson reviewed Nov 9, 2017
View reviewed changes
parser.jison
@@ -72,6 +73,7 @@ expression
| css_variable { $$ = $1; }
| css_value { $$ = $1; }
| value { $$ = $1; }
| UNKNOW_FUNCTION { $$ = { type: 'Unknow', value: $1 }; }
Copy link
Contributor

Choose a reason for hiding this comment

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

If the name is to be kept, it should be "unknown function". But I'm leaning towards normalisation, so maybe just "function".

@MoOx
Copy link
Owner

MoOx commented Sep 20, 2018

I just saw this pr after merging another workaround. Is this PR still relevant?

@MoOx
Copy link
Owner

MoOx commented Sep 20, 2018

(workaround have constant and env hardcoded, see #48)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andyjansson andyjansson andyjansson left review comments

@ben-eb ben-eb Awaiting requested review from ben-eb

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Constant values not supported
3 participants
@Semigradsky @MoOx @andyjansson