Extend PythonExpression substitution to simplify the notation #600
Conversation
Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>
Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>
kenji-miyake
force-pushed
the
extend-python-expression
branch
from
06b36f0
to
f5c41b2
Compare
|
I extended |
|
@ivanpauno Hello, would you kindly review this, please? 🙏 |
| return str(eval(perform_substitutions(context, self.expression), {}, math.__dict__)) | ||
| expressions = [perform_substitutions(context, [exp]) for exp in self.expression] | ||
|
|
||
| if len(expressions) == 3 and (expressions[1] in ['==', '!=']): |
There was a problem hiding this comment.
@kenji-miyake I understand it's the easy path forward, but we can't merge a barely-implemented feature. IMHO, if we want $(eval) to be able to use launch configurations as variables, we need to inject launch configurations in eval()'s local scope.
There was a problem hiding this comment.
we need to inject launch configurations in eval()'s local scop
Yeah that sounds better, the current proposal is too ad-hoc.
I'm not sure whether I should support $(eval value == 'abc') as well, but I guess it's a bit difficult because the current system seems to automatically drop ''.
Maybe this is a problem (?).
We probably need to modify the launch substitutions grammar to make it more flexible, which will require a bit of thinking.
There was a problem hiding this comment.
Thank you for your reviews!
we need to inject launch configurations in eval()'s local scop
Is there any sample code that does this? Unfortunately, I'm not so familiar with the design concept of launch. 😢
Anyway, I'll drop expanding LaunchConfiguration from this PR for now.
There was a problem hiding this comment.
Reverted by 4cc38be.
@hidmic @ivanpauno Would you please teach me what else should I change? 🙏
There was a problem hiding this comment.
@hidmic @ivanpauno Friendly ping. Could you review this again? 🙇
There was a problem hiding this comment.
@hidmic Thank you for your comment!
What if an expression is supposed to expand to, the name of some constant like pi?
Hmm, I didn't care about the case. But I guess just adding an eval with try-cache can support it? 🤔
IMO to have this feature we need to revisit that grammar.
Yes, I agree with that. (I've changed this to a draft for now.)
How can I build a consensus on the syntax and move things forward?
Are there any design concept documents or plans for the eval syntax?
There was a problem hiding this comment.
Or, how about adding eq and ne substitutions like #598?
$(eq $(var foo) 'bar')
$(ne $(var foo) 'bar')
There was a problem hiding this comment.
@kenji-miyake first of all, my apologies for the insane delay. I've been uber busy.
Now, circling back to this.
How can I build a consensus on the syntax and move things forward?
A concrete proposal is the way to go. You want to simplify notation by making $(var name) and name equivalent in $(eval ...) context. We agree with the intent, but we need a fully functional implementation i.e. not just for a few operators. One possible approach is to inject launch configurations in eval scope.
That doesn't solve the quoting issue though. In launch substitutions' grammar, arguments are delimited by spaces and so you need quotes to pass a single argument that includes a space e.g. '/my/very/unhelpful path'. Quotes must be dropped in these cases. When an $(eval) expression is not quoted, the parser will try to split expression into space delimited arguments, potentially dropping quotes from string literals, and the rest follows.
The only easy way I see we could avoid this is by instructing the substitution parser to skip the entire $(eval) expression. That means, no nested substitutions, tracking left and right parenthesis, and coming up with a special escaping mechanism for parenthesis so that something like $(anon $(eval data() + ')')) doesn't fall apart.
Are there any design concept documents or plans for the eval syntax?
IIRC there are no design documents about the syntax itself. Only the grammar.
how about adding
eqandnesubstitutions
That's definitely a simpler (better?) path forward.
There was a problem hiding this comment.
@hidmic Hi, welcome back! No problem, I know ROS maintainers are quite busy. 😢
And thank you for your detailed explanation. But it's super difficult for me... I have to learn the grammar a lot.
That's definitely a simpler (better?) path forward.
Oh, it's good to hear that. Can I send a new PR to add eq/ne?
There was a problem hiding this comment.
eq/ne seem to be implemented in #649.
This reverts commit f5c41b2. Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>
kenji-miyake
force-pushed
the
extend-python-expression
branch
from
4cc38be
to
83e059e
Compare
| if len(data) != 1: | ||
| raise TypeError('eval substitution expects 1 argument') | ||
| return cls, {'expression': data[0]} | ||
| if len(data) == 1: |
There was a problem hiding this comment.
@kenji-miyake we should still catch len(data) == 0 and raise an exception.
There was a problem hiding this comment.
Fixed in 205e7cc.
And added a test.
| return str(eval(perform_substitutions(context, self.expression), {}, math.__dict__)) | ||
| expressions = [perform_substitutions(context, [exp]) for exp in self.expression] | ||
|
|
||
| if len(expressions) == 3 and (expressions[1] in ['==', '!=']): |
There was a problem hiding this comment.
@kenji-miyake my apologies for the delay. I still think this is special casing to workaround the fact that, as @ivanpauno pointed out, the substitution grammar wasn't designed to deal with arbitrary Python expressions. What if an expression is supposed to expand to, the name of some constant like pi? That implicit quoting you're adding in line 79 would block that.
IMO to have this feature we need to revisit that grammar. And that's separate from simplifying $(var name) to name when used within $(eval ...).
Signed-off-by: Kenji Miyake <kenji.miyake@tier4.jp>
kenji-miyake
force-pushed
the
extend-python-expression
branch
from
d4135bf
to
205e7cc
Compare
|
I believe I can close this thanks to #649. |
Related to #469, I supported the notation of
$(eval $(var value) == 'abc').I'm not sure whether I should support
$(eval value == 'abc')as well, but I guess it's a bit difficult because the current system seems to automatically drop''.I'll send this Draft PR in order to get reviews from maintainers.