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

Timelion grammar needs to strictly require comma-delimitation between expressions. #10907

Closed
cjcenizal opened this issue Mar 27, 2017 · 3 comments · Fixed by #10961
Closed

Timelion grammar needs to strictly require comma-delimitation between expressions. #10907

cjcenizal opened this issue Mar 27, 2017 · 3 comments · Fixed by #10961
Assignees
Labels
blocker Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:enhancement

Comments

@cjcenizal
Copy link
Contributor

cjcenizal commented Mar 27, 2017

Blocks #8881.

Timelion expressions are currently both comma- and space-delimited. This precludes the possibility for writing multiline expressions and using whitespace as an organizational tool. For instance:

.es(*)
  .add(10)
  .multiply(20)

.es(*)
  .add(5)
  .multiply(10)

Will be interpreted as:

.es(*).add(10).multiply(20).es(*).add(5).multiply(10)

This is an invalid expression. To support the organizational use of whitespace in multiline expressions, we need to update Timelion's grammar to strictly require comma-delimitation between expressions, and ignore all whitespace.

Note: @rashidkpc mentioned that the ability to space-delimit your expressions is undocumented, so its usage should be less popular than the use of comma-delimited expressions. Nevertheless, we should display a warning if a user's expression is invalid and explain this change as a possible cause of the problem. @tbragin We'll also need to notify users of this potentially BWC breaking change in our release notes.

CC @thomasneirynck @ppisljar

@cjcenizal cjcenizal added Feature:Visualizations Generic visualization features (in case no more specific feature label is available) blocker release_note:enhancement labels Mar 27, 2017
@cjcenizal cjcenizal self-assigned this Mar 27, 2017
@ppisljar
Copy link
Member

will it ? this query seems to work correctly for me .es( 404 ) .es(*).yaxis( min : 10)

@ppisljar
Copy link
Member

with a minor change i can make this one work as well: (spaces before ():

.es     (    404   )                  .es(*).yaxis    (     min    :   10)

@ppisljar
Copy link
Member

ah ok .... i put space everywhere expect where it causes problem :)

.es .yaxis(min:10) will break

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants