Feat: Variable parsing rework (Array input) #1200
Conversation
No context yet, so error information is not helpful, as well as some parsing is broken/major change
mostly done with quoted string parser, still a few gotcha cases like `'abc\'"', but it should be simple to fix that.
|
Can you add a few examples of how successful input looks like? What if you have an array of tuples which have arrays in them? |
|
We definitely shouldn't be including things like |
|
We need to figure out how to make check warnings not appear once for each platform |
|
(Get rid of all the unused imports) |
Array of tuples
Array of tuples with arrays
Array of json
|
…db-cli into feat/variable-parsing-rework
src/cloud/ops.rs
Outdated
| @@ -85,19 +98,49 @@ pub struct Version { | |||
| pub version: String, | |||
| } | |||
|
|
|||
| #[derive(Debug, serde::Deserialize)] | |||
There was a problem hiding this comment.
hm..., weird? why is github including this in the diff?
|
Added named tuples, might wanna gloss over it |
|
Oh, clean up the unused imports please |
|
Do the tuples and array support trailing commas? They should, and we should make sure to test it |
No, any extra comma errors. I'll change that |
|
Hm, actually now that I think about it more, maybe we should require the named tuples to be in order, also? That matches edgeql better (where the order is important), though (for example) the Python bindings will accept dictionaries and namedtuples without caring about order. @elprans thoughts on this? We should probably also support writing non-named tuples when a named tuple is expected also, which I now realize is what @elprans was probably saying earlier |
|
Oh, and leading spaces after an opening |
this will be exclusive right? ex: cant have 1 named and 1 unnamed part; its either all named or no named? |
Yeah |
| delimited(multispace0, f, multispace0) | ||
| } | ||
|
|
||
| fn space(i: &str) -> IResult<&str, &str, ParsingError> { |
There was a problem hiding this comment.
This is the same as multispace0, right?
src/prompt/variable.rs
Outdated
| if result.len() < self.shape.elements.len() { | ||
| return Err(ParsingError::Incomplete { | ||
| hint: Some( | ||
| format!( | ||
| "Expecting one of the following element name(s): {}", | ||
| self.shape.elements.iter() | ||
| .filter(|v| !result.iter().any(|t| t.0 == v.name.as_str())) | ||
| .map(|v| v.name.clone()) | ||
| .collect::<Vec<String>>() | ||
| .join(", ") | ||
| ) | ||
| ) | ||
| }) |
There was a problem hiding this comment.
I think we need to handle the case with duplicate field names also
|
I just pushed commits that support trailing commas for tuples, named tuples, and arrays. Mostly I did this as a personal experiment with The combinator-y tuple that works by building up a new boxed parser isn't too bad. It took some fiddling because I'm not that good at rust anymore, but is basically reasonable, I think. The array/namedtuple version I found very frustrating. I implemented a function |
|
@quinchs what's the status of this? |
I've got some remaining changes to fix with it which sully has mentioned. |
Summary
This PR reworks the variable input mechanism to use
nomas a parser; allowing us to parse array inputs and other complex inputs.Array Inputs
The array input works much like writing an array in EdgeQL, as proposed by Sully:
and it also forces
strto be quoted with either single quotes or double quotes in the input:Error reporting and hints
With the change to nom, errors and hints have changed too as a side effect, in the current state of this PR it dumps the parser errors pretty verbosely, I would like some feedback on how we should format these errors to be as concise as possible, some examples:
Things get pretty messy with some of the parsers though:
