feat(parser): remove the impl Display for generated nodes #330
Conversation
| @@ -26,7 +26,7 @@ impl From<Description> for String { | |||
| #[cfg(feature = "parser-impl")] | |||
| impl From<apollo_parser::ast::Description> for Description { | |||
| fn from(desc: apollo_parser::ast::Description) -> Self { | |||
| Description(StringValue::from(desc.to_string())) | |||
| Description(desc.string_value().map(|s| s.into()).unwrap_or_else(|| StringValue::Line(Default::default()))) | |||
There was a problem hiding this comment.
I think this was passing the quoted string instead of the string contents previously, turning "text" into """"text""""
There was a problem hiding this comment.
ah, yea the original line should have been desc.text().to_string(), since .text() strips away the extra quotation marks around string values. So that's something to keep in mind with this PR - source_string() for string values that have quotes around them should have those quotes stripped.
| #[cfg(feature = "parser-impl")] | ||
| impl From<apollo_parser::ast::StringValue> for StringValue { | ||
| fn from(val: apollo_parser::ast::StringValue) -> Self { | ||
| Self::from(Into::<String>::into(val)) |
There was a problem hiding this comment.
This Into::<String>::into() business is a bit awkward, i wonder if we could impl From<StringValue> for String instead? or provide a separate named method that returns the unquoted contents of a StringValue?
There was a problem hiding this comment.
We already have impl Into<String> for ast::StringValue here.Froms are usually nicer, but that requires us to create a syntax node with an unknown source, which perhaps is a bit too tricky? What do you think?
There was a problem hiding this comment.
I think we need a few tests to go along with this in the parser side of things. I mostly want to make sure we know if this is going to be a breaking change, for example #98 still works etc. i think a few tests for a some of node conversions, as well as making sure we can still properly display and have the source string of the whole tree (and it's equivalent to the original input). What do you think?
|
Maybe the Probably better for a separate PR though 😇 |
|
hmmm you mean the other way around? Our impl Into<String> for ast::StringValue {
fn into(self) -> String {
//
}
}In your test you can't use the (separate PR is good!) |
|
Yes i mean that's what it does, but the implementation of the function doesn't actually require ownership, it only takes ownership to satisfy the trait. The same feature could be implemented as a |
goto-bus-stop
changed the title
As mentioned in #329 (comment)
The
Displayimpls for generated nodes returned the source text for that node. That's not a super common operation but it was very easy to access. It's also a very different operation from eg.let content: String = node.string_value().into()which returns the content of a string:node.string_value().to_string()returned the string as it was written in the source code, quotes and escapes and all.Now
.to_string()is replaced by a.source_string()method. It allocates a new String (just like.to_string()did). A syntax node can represent multiple slices (I think to support different structures like Ropes as input?), so slicing the original source isn't actually possible.