List/dict syntax when using @{list} and &{dict} as embedded arguments
#3218
Conversation
…s embedded arguments
There was a problem hiding this comment.
On a quick look most of the code looks pretty good and it's great to see tests and docs included. The actual code for converting values to lists/dicts has various issues mentioned in a separate comment, though. In addition to that, there doesn't seem to be code (or tests) for this feature with library keywords.
| yield n, variables.replace_list(v) | ||
| except: | ||
| pass | ||
| yield n, variables.replace_scalar(v) |
There was a problem hiding this comment.
Blindly using eval and especially empty except: are both pretty ugly. Also, wouldn't this code turn embedded argument values that can be evaluated in Python to the resulting value? For example, arguments like True and 1 + 2 (both strings) would be converted to Python True and integer 3.
Evaluation should only be done if the original argument name starts with @ or &. Then it should be validated that if it starts with @, the result should be a list, and if it starts with & it should be a dict. Also, evaluation should rather be done using ast.literal_eval than eval.
It would probably be a good idea to implement dedicated utility functions for converting string to lists and dicts. In the beginning these utils could just use ast.literal_eval (and validate the result is list/dict), but in the future we could try coming up with other, more readable formats as discussed in #2699.
|
I commited the changes I had in progress for this PR in 34b6825. The code is still not finished yet, but if it's decided at some point in the future that this is needed I will continue the implementation. |
PR for #2699