hoaproject / Ruler

The Hoa\Ruler library.
https://hoa-project.net
627 stars 66 forks source link

Space or no-space in expressions #115

Open beberlei opened 6 years ago

beberlei commented 6 years ago

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 commented 6 years ago

Hello :-),

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

beberlei commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago
$ 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 commented 6 years ago

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

Hywan commented 6 years ago

The trick is easy actually:

https://github.com/hoaproject/Ruler/blob/28174caf3be01144e7e213fce75598404570e094/Grammar.pp#L67

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 commented 6 years ago

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 commented 6 years ago

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

Hywan commented 6 years ago

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.