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

WHERE leading whitespace #348

Open
flip111 opened this issue Feb 7, 2019 · 4 comments
Open

WHERE leading whitespace #348

flip111 opened this issue Feb 7, 2019 · 4 comments

Comments

@flip111
Copy link

flip111 commented Feb 7, 2019

I looked around where the WHERE rule was being used. In all places except one it's used in the EBNF as [[SP], Where]. The place where it's not used with the leading SP (whitespace) is here

<opt><non-terminal ref="Where"/></opt>

When following StartPoint (the line before, 46):

  • StartPoint ends with Lookup
  • Lookup ends with NodeLookup or RelationshipLookup
  • NodeLookup and RelationshipLookup end with IdentifiedIndexLookup or IndexQuery or IdLookup
  • IdentifiedIndexLookup and IndexQuery and IdLookup all end with token )

That's strange that in this case ) must be immediately followed with token WHERE without whitespace in between. Maybe this is a mistake in the grammar? In case the grammar would change to also put whitespace in this location, i would suggest to put the whitespace in the WHERE rule itself.

Old rule: Where = (W,H,E,R,E), SP, Expression ;
New rule: Where = SP, (W,H,E,R,E), SP, Expression ;

@Mats-SX
Copy link
Member

Mats-SX commented Feb 12, 2019

I believe you've found a bug. I agree with the proposed change. I'd be happy to review a PR if one appears, otherwise we'll pick this up at some point.

Thanks for the report!

@thobe
Copy link
Contributor

thobe commented Feb 12, 2019

I have a slight preference for inserting the whitespace at the point of reference rather than into the production rule for Where. But we have been talking about removing the explicit whitespace referenced in the source, and instead have the generators inject it based on rules about the structure of the grammar.

@Mats-SX
Copy link
Member

Mats-SX commented Feb 12, 2019

Just to clarify, I don't feel strongly on where SP is declared (at reference vs at definition; I tend to favour reference but definition means fewer occurrences), but I do believe it is still a bug that it isn't present on the referenced line (47 in start.xml). Quickest bugfix would probably be to make that say

<opt> &SP; <non-terminal ref="Where"/></opt>

I would be in favour of removing the explicit whitespace declarations in favour of general rules, if those rules could be formulated succinctly without lots of special cases.

@flip111
Copy link
Author

flip111 commented Feb 12, 2019

Maybe from a grammar design perspective i would also prefer to have the whitespace at the point of reference. After all "a SP b SP c" is easier to read than "a SP b c" where c starts with SP.

But on the other hand (for my purpose), i'm working on letting the user supply nodes in the grammar (like a querybuilder). Now for each concatenation/sequence i have a node in the grammar. But the concatenation [[SP], Where] has no meaning. It doesn't even have an explicit name like rules because it's used as part of another rule. That's why the moment i have an "optimization step" that goes like this: when i have a concatenation of two and the first item is a literal of SP then move this SP into the second item. Right now this rule also gets triggered in other places maybe i get back to those later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants