Skip to content
This repository has been archived by the owner on Sep 20, 2021. It is now read-only.

Space or no-space in expressions #115

Open
beberlei opened this issue Jan 31, 2018 · 10 comments
Open

Space or no-space in expressions #115

beberlei opened this issue Jan 31, 2018 · 10 comments

Comments

@beberlei
Copy link

It is a difference to have foo="bar" or foo = "bar" when evaluating this grammar. Is there a way to have both the same?

@Hywan
Copy link
Member

Hywan commented Jan 31, 2018

Hello :-),

I don't think there is any difference. Both should be supported.

@beberlei
Copy link
Author

beberlei commented Jan 31, 2018

@Hywan thants for the quick reply.

It renders different models var_dump((string)$model); from interpret():

$model->expression =
    $model->{'='}(
        $model->variable('method'),
        'POST'
    );
$model = new \Hoa\Ruler\Model();
$model->expression =
    $model->variable('method="POST"');

@beberlei
Copy link
Author

@Hywan i should mention, we are not using ruler to execute, but we are visiting the AST ourselves to convert it into Elasticsearch query.

@Hywan
Copy link
Member

Hywan commented Jan 31, 2018

Indeed, I can reproduce the bug. I don't have right now to fix it, but probably on Friday. Is it working for you?

@Hywan
Copy link
Member

Hywan commented Jan 31, 2018

$ echo 'foo = "bar"' | vendor/bin/hoa compiler:pp vendor/hoa/ruler/Grammar.pp 0 -s
 #  namespace     token name            token value                     offset
-------------------------------------------------------------------------------
 0  default       identifier            foo                                  0
 1  default       identifier            =                                    4
 2  default       string                "bar"                                6
 3  default       EOF                                                       12

$ echo 'foo="bar"' | vendor/bin/hoa compiler:pp vendor/hoa/ruler/Grammar.pp 0 -s
 #  namespace     token name            token value                     offset
-------------------------------------------------------------------------------
 0  default       identifier            foo="bar"                            0
 1  default       EOF                                                       10

I suppose I know how to fix that.

@beberlei
Copy link
Author

@Hywan don't worry or feel the need to commit to a time frame, i have a "workaround" for now :-)

@Hywan
Copy link
Member

Hywan commented Jan 31, 2018

The trick is easy actually:

%token identifier [^\s\(\)\[\],\.]+

We must change the token to be:

- %token  identifier    [^\s\(\)\[\],\.]+
+ %token  identifier    [^\s\(\)\[\],\.=]+

We should include also common operators. That's because an identifier can be anything.

@beberlei
Copy link
Author

beberlei commented Feb 1, 2018

I tried to copy the grammar with the small change, but it leads to a new error:

Unrecognized token "=" at line 1 and column 9:
release = 1

@Hywan
Copy link
Member

Hywan commented Feb 5, 2018

Sorry, had to fix other higher priority bugs. I'll come back to you as soon as possible :-).

@Hywan
Copy link
Member

Hywan commented Feb 5, 2018

Sorry, had to fix other higher priority bugs. I'll come back to you as soon as possible :-).

Try with:

%token identifier %token  identifier    ((=|!=|>|>=|<|<=)|[^\s\(\)\[\],\.=!<>]+)

The problem is that both the operands and the operator are represented by the identifier token. To be the most flexible and free possible, the space is the main separator between the tokens. I never thought about your usecase, and yes, it can be annoying. The identifier is slightly modified to cut over =, !=, >, >=, <, and <=. I don't like being specific in this case, it looks like a hack, but I guess we have no choice.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

No branches or pull requests

2 participants