edgedb-cli icon indicating copy to clipboard operation
edgedb-cli copied to clipboard

Feat: Variable parsing rework (Array input)

Open quinchs opened this issue 1 year ago • 17 comments

Summary

This PR reworks the variable input mechanism to use nom as 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:

cli> select <array<int64>>$arg;
Parameter <array>$arg: [1, 3, 5]
{[1, 3, 5]}

and it also forces str to be quoted with either single quotes or double quotes in the input:

cli> select <array<str>>$arg;
Parameter <array>$arg: ["abc", 'def']
{['abc', 'def']}

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: image image image image

Things get pretty messy with some of the parsers though: image

quinchs avatar Jan 31 '24 16:01 quinchs

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?

1st1 avatar Jan 31 '24 18:01 1st1

We definitely shouldn't be including things like TakeWhileMN in error messages

msullivan avatar Jan 31 '24 19:01 msullivan

We need to figure out how to make check warnings not appear once for each platform

msullivan avatar Jan 31 '24 19:01 msullivan

(Get rid of all the unused imports)

msullivan avatar Jan 31 '24 22:01 msullivan

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?

Array of tuples

image

Array of tuples with arrays

image

Array of json

image

quinchs avatar Feb 07 '24 17:02 quinchs

The big remaining thing needed is tests. There probably ought to have been some tests before, but now the parsing is much more involved so we definitely need them.

msullivan avatar Feb 07 '24 23:02 msullivan

Added named tuples, might wanna gloss over it

quinchs avatar Feb 13 '24 00:02 quinchs

Oh, clean up the unused imports please

msullivan avatar Feb 13 '24 02:02 msullivan

Do the tuples and array support trailing commas? They should, and we should make sure to test it

msullivan avatar Feb 13 '24 02:02 msullivan

Do the tuples and array support trailing commas?

No, any extra comma errors. I'll change that

quinchs avatar Feb 13 '24 18:02 quinchs

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

msullivan avatar Feb 13 '24 19:02 msullivan

Oh, and leading spaces after an opening ( or [

msullivan avatar Feb 13 '24 21:02 msullivan

We should probably also support writing non-named tuples when a named tuple is expected

this will be exclusive right? ex: cant have 1 named and 1 unnamed part; its either all named or no named?

quinchs avatar Feb 13 '24 22:02 quinchs

We should probably also support writing non-named tuples when a named tuple is expected

this will be exclusive right? ex: cant have 1 named and 1 unnamed part; its either all named or no named?

Yeah

msullivan avatar Feb 13 '24 22:02 msullivan

I just pushed commits that support trailing commas for tuples, named tuples, and arrays. Mostly I did this as a personal experiment with nom.

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 trailing_separated_list0, with the intention that it behave like separated_list0 but allow trailing separators. This worked but I wasn't able to figure out a way to do it purely by composing combinators, and needed to invoke the parsers directly, which cuts against the benefit of parser combinators somewhat. The final result isn't actually that bad, though, just unsatisfying.

msullivan avatar Feb 21 '24 23:02 msullivan

@quinchs what's the status of this?

aljazerzen avatar Apr 18 '24 15:04 aljazerzen

@quinchs what's the status of this?

I've got some remaining changes to fix with it which sully has mentioned.

quinchs avatar Apr 18 '24 16:04 quinchs