apollo-rs icon indicating copy to clipboard operation
apollo-rs copied to clipboard

should StringValue nodes contain the quotes?

Open Geal opened this issue 3 years ago • 1 comments

Description

I see that when parsing a string value, the node's span contains the double quotes. Example here: https://github.com/apollographql/apollo-rs/blob/main/crates/apollo-parser/src/parser/grammar/value.rs#L208

So when manipualting it, if I want to access the text, I need to remove them. It is not a big issue, but I was wondering if that is something that could be done at the API level (a method of StringValue removing the quotes) or at the parsing level (considering the quotes bound the string token but are not part of it).

Geal avatar Oct 26 '21 13:10 Geal

So according to the spec, StringValue does have quotes. This means that it would be incorrect for us to remove them at the parsing level. Quotation marks are also not considered tokens, so I would not be able to set up a structure where a StringValue can be represented as a series of quotation mark tokens an ident atom.

It may be possible to have a text() fn for StringValue to remove quotation marks. We must, however, keep all indentation and whitespace that's contained between quotation marks. Let me think a little bit about whether or not this would be actually be considered correct, or if we are misrepresenting the spec by omitting quotes.

lrlna avatar Oct 26 '21 16:10 lrlna

in which PR (and version) was it fixed? I'll check if there's still some code in the router remaining with quotes handling

Geal avatar Sep 16 '22 13:09 Geal

oops, closed without submitting a comment! apologies

we implemented a Into<String> for StringValue a while back as part of #111 and published way before we started doing a changelog (so pre 0.2.0). you will already have this in the router.

here's how you'd use it:

if let ast::Value::StringValue(val) = argument.value().expect("Cannot get argument value.") {
    let val: String = val.into(); // trims `"`
}

lrlna avatar Sep 16 '22 13:09 lrlna