jingoo icon indicating copy to clipboard operation
jingoo copied to clipboard

default filter is not usable

Open sagotch opened this issue 4 years ago • 8 comments

This is a regression introduced by https://github.com/tategakibunko/jingoo/commit/8d1073cbc881933192821b31991ef0607dfa4bbb

Because default might be a token, it is returned as a token whenever you are in a Logic lexer_mode.

{% set pg = default (0, env.pg) %}

will raise

Jingoo.Jg_types.SyntaxError("Error line _, col _, token default ()")

I will take a look at it when I have time.

sagotch avatar Sep 14 '20 14:09 sagotch

Ok, we tested default filter only as jg_default.

I've added example/samples/filter_default.{jingoo, expected}.

tategakibunko avatar Sep 15 '20 09:09 tategakibunko

Fortunately, the only difference between default(in switch case statement) and default(as filter) is that only latter is followed by LPAREN.

So I've treated the default filter as special ApplyExpr in jg_parser.mly.

https://github.com/tategakibunko/jingoo/commit/ddb49e4d74192a904a6961a59245dc773ece0b6e

All test work fine, but if there is something wrong, point it to me.

tategakibunko avatar Sep 17 '20 10:09 tategakibunko

Hmm, my fix doesn't work in set statement...

{{ set baz = default('baz', null) }}

It causes

Jingoo.Jg_types.SyntaxError("Error line 3, col 6, token set ()")

tategakibunko avatar Sep 17 '20 11:09 tategakibunko

Sorry, with my quick fix, we'll have to fix everything where ApplyExpr comes up in jg_parlser.mly.

This fix doesn't look good, so I'll revert it.

EDIT:

Oh, my example code was wrong! After fixing, test works fine!

-{{ set baz = default('baz', null) }}{{ baz }}
+{% set baz = default('baz', null) %}{{ baz }}

https://github.com/tategakibunko/jingoo/commit/ba4de0ce4073a58c394af7520ad09fefc31f2ecf

tategakibunko avatar Sep 17 '20 11:09 tategakibunko

Seems a little bit hackish but if it works it is better than without the hack, thanks for taking care of this :+1:

But this issue highlight another question:

Should jingoo:

  1. rely on a reserved keywords list
  2. or should we be able to define some macro/functions which use a name wich is a keyword otherwise.

i.e. should this be allowed?

{% function switch (b) %}{{ not b }}{% endfunction %}

{{ switch (true) }}

I think that 2 would be preferable and should be pretty easy to achieve but I did not try to implement it (yet?).

sagotch avatar Sep 17 '20 12:09 sagotch

Okay, apparently this problem wasn't as easy as I thought it would be.

It would certainly be better to be able to use keywords in functions, macro.

But to use keyword like switch, default in various context, maybe we have to find out whether the token is keyword or identifier in the lexer context after all, but it's too hard task for me.

By the way, the reason I've added this fix in a rough way is that I'm using default filter myself in my web site, haha.

(And maybe many devs are using default filter because it's a useful filter)

tategakibunko avatar Sep 17 '20 12:09 tategakibunko

I'd like to be able to declare functions that is using keyword in the future, but the default filter seems to have a big impact, so for now, I'm going to release this fix (although which isn't very good).

When you have time, feel free to remove my fix, and implement your own!

tategakibunko avatar Sep 17 '20 13:09 tategakibunko

(And maybe many devs are using default filter because it's a useful filter)

That's how I found the bug :)

I'd like to be able to declare functions that is using keyword in the future, but the default filter seems to have a big impact, so for now, I'm going to release this fix (although which isn't very good).

When you have time, feel free to remove my fix, and implement your own!

I'll try to implement a general fix :+1:

sagotch avatar Sep 17 '20 14:09 sagotch