Allow '$' character in key names in the grammar #600
Conversation
|
|
||
| // Interpolation key, may contain any non special character. | ||
| // Note that we can allow '$' because the parser does not support interpolations that | ||
| // are only part of a key name, i.e., "${foo${bar}}" is not allowed. As a result, it | ||
| // is ok to "consume" all '$' characters within the `INTER_KEY` token. | ||
| INTER_KEY: ~[\\{}()[\]:. \t'"]+; |
There was a problem hiding this comment.
Side question:
Why are we allowing whitespace in interpolation keys?
There was a problem hiding this comment.
They are not allowed (what we see here is the set of forbidden characters, and " " is in it)
omegaconf/grammar_parser.py
Outdated
| _config_key = f"({_id}|\\$)+" # foo, $bar, $foo$bar$ | ||
| _dot_path_res = f"({_id}(\\.{_id})*)?" # foo, foo.bar3, foo_.b4r.b0z | ||
| _dot_path_node = f"(\\.)*({_config_key}(\\.{_config_key})*)?" # foo, .foo.$bar | ||
| _inter_node = f"\\${{\\s*{_dot_path_node}\\s*}}" # node interpolation | ||
| _arg = "[a-zA-Z_0-9/\\-\\+.$%*@]+" # string representing a resolver argument | ||
| _args = f"{_arg}(\\s*,\\s*{_arg})*" # list of resolver arguments | ||
| _inter_res = f"\\${{\\s*{_dot_path}\\s*:\\s*{_args}?\\s*}}" # resolver interpolation | ||
| _inter_res = f"\\${{\\s*{_dot_path_res}\\s*:\\s*{_args}?\\s*}}" # resolver interp. |
There was a problem hiding this comment.
This is becoming quiet complex.
Why do we need both _dot_path_res and _dot_path_node?
res stands for resolver? if so, rename it - this is very confusing. (specially how the definition for it is intertwined with the dotpath definition.
There was a problem hiding this comment.
Yes, "res" stands for "resolver". I was trying to make sure it would fit in a single line because it gets worse when black starts making these multi-line. I tried to use better names and a more logical definition order in c3403df.
We need both _node_path and _resolver_path (new names in that commit) because we want to allow $ in node names, but not in resolver names.
omegaconf/grammar_parser.py
Outdated
| _inter_node = f"\\${{\\s*{_dot_path_node}\\s*}}" # node interpolation | ||
| _node_path = f"(\\.)*({_config_key}(\\.{_config_key})*)?" # foo, .foo.$bar | ||
| _node_inter = f"\\${{\\s*{_node_path}\\s*}}" # node interpolation ${foo.bar} | ||
| _resolver_path = f"({_id}(\\.{_id})*)?" # foo, ns.bar3, ns_1.ns_2.b0z |
There was a problem hiding this comment.
| _resolver_path = f"({_id}(\\.{_id})*)?" # foo, ns.bar3, ns_1.ns_2.b0z | |
| _resolver_name = f"({_id}(\\.{_id})*)?" # foo, ns.bar3, ns_1.ns_2.b0z |
There was a problem hiding this comment.
Done in d70a31f (including the corresponding rename down below)
No functional change
Co-authored-by: Omry Yadan <[email protected]>
Co-authored-by: Omry Yadan <[email protected]>
Co-authored-by: Omry Yadan <[email protected]>
odelalleau
force-pushed
the
dollar_in_key
branch
from
d70a31f to
49b2e56
Compare
This is a first step towards addressing facebookresearch/hydra#1437
Best reviewed by individual commits