Tuple and array arguments supported in resim
#1727
Conversation
|
Docker tags |
Benchmark for 8ad8361Click to view benchmark
|
There was a problem hiding this comment.
This is really cool! :D
Side thoughts:
- This is not the standard approach to implementing a parser (normally as per manifest land, you'd implement a lexer and parser) which is why there are quite a lot of edge cases. But the code's a lot more compact. I don't think it's a problem and I don't think it needs changing; but I think it could do with a few of the edge cases being neatened out, and documented a little more.
- A part of me wonders if we should accept manifest syntax or programmatic json or something (where we have proper lexers already) but I think I prefer your approach tbh :)
Separately, I wonder if we should detect a well known type id of NonFungibleGlobalId, and allow that to be provided as the canonical resource_address:local_id string (as well as the tuple)?
|
|
||
| // Splits argument tuple or array elements delimited with "," into vector of elements. | ||
| // Elements with brackets (round or square) are taken as is (even if they may include ",") | ||
| fn split_argument_tuple_or_array(argument: &str) -> Result<Vec<String>, BuildCallArgumentError> { |
There was a problem hiding this comment.
After the below fix, there are still some constraints around what can be parsed:
- Strings anywhere inside tuples or arrays can't contain
[or]or(or). - Strings directly inside tuples or arrays can't start/end with whitespace.
This isn't necessarily a problem. But we probably want to add the new syntax to the resim documentation somewhere and explain its limitations?
There was a problem hiding this comment.
Although I wonder if we should consider adding some string support with "? Both to the below parser, and to the string parser which would discard start and stopping " -- i.e. we could have an "is in string" flag, and avoid interpreting any other characters until we see a ".
There was a problem hiding this comment.
Yeah, surrounding strings with " would make things easier, but it would be quite different of other places in resim, where string could be supplied. Not sure if want it...
There was a problem hiding this comment.
I guess I was thinking that it could be optional, so abc, "abc" and "abc [ asda" are all acceptable for a string. But it does further complicate the splitter/parser!
| let mut chars = argument.chars(); | ||
|
|
||
| while let Some(c) = chars.next() { | ||
| if c.is_whitespace() { |
There was a problem hiding this comment.
This discards all whitespace inside strings contained inside tuples and arrays, I think this is a little too heavy handed, e.g. (hello world,123) would parse as ("helloworld", 123)
There was a problem hiding this comment.
I think instead we should have the check contingent on round_brackets_level == 0 && square_brackets_level == 0
There was a problem hiding this comment.
This discards all whitespace inside strings contained inside tuples and arrays, I think this is a little too heavy handed, e.g.
(hello world,123)would parse as("helloworld", 123)
Good point 🤦
| } | ||
| _ => match prev_c { | ||
| Some(prev_c) if prev_c == ']' || prev_c == ')' => { | ||
| return Err(BuildCallArgumentError::FailedToParse(format!( |
There was a problem hiding this comment.
This allows (hello)(world) (with no comma) to mean "(hello)(world)" which feels a little wrong, but on reflection, I don't think it's necessarily a problem...
There was a problem hiding this comment.
I think comma should be mandatory, so would prefer to fix it.
This is exactly what I was thinking. In fact I've implemented it in the first commit of this PR, but I replaced it with Tuple and Array approach. But I believe it would be really nice to support it as well. |
Summary
Support for passing Tuple and Array values as function/method arguments in
resim.Tuple arguments shall be surrounded with round brackets
(and).Array arguments shall be surrounded with square brackets
[and].Examples:
ResourceAddress,NonFungibleLocalIdNested Tuples/Arrays are also possible :)