Store spans for Value expressions#1738
Conversation
|
feel free to edit the code directly without asking me first :) |
|
FYI @eliaperantoni -- any chance you are willing to help our with this one? |
eliaperantoni
left a comment
eliaperantoni
left a comment
There was a problem hiding this comment.
Looks awesome! This is a much needed feature, thank you for the contribution :) The library changes look good to me, I will help with the tests as soon as I can
|
I did the bigquery tests. The others are remaining
If someone wants to volunteer help... |
I will post a note in DataFUsion slack and see if I can find anyone to help In terms of logitistics, I suggest people:
I will do an example of this for one or two in a moment |
|
@lovasoa here is a proposed contribution: If you like that, I think we can apply the same thing to the other tests fairly easily |
|
Ok, ast-grep is a bit of a pita to use with rust, but I managed to get it to do most of the work with the following rule: id: x
language: rust
rule:
any:
- kind: token_tree
pattern: $ARG
follows:
kind: identifier
pattern: Value
follows:
kind: identifier
pattern: Expr
stopBy: end
fix: ($ARG.with_empty_span())the tests pass now ! |
|
Looking quite nice! Any chance you can pull the doc strings from lovasoa@4d101b0 as well? |
I did. I think this is ready for merging |
|
It appears there are a few failed tests:
|
😬 I had forgotten to run |
lovasoa
changed the title
|
It would be nice if we could get this merged before other pending PRs, because it touches almost all the tests, and is guaranteed to generate big conflicts as we change them. |
| value: Box::new(Expr::Value(Value::SingleQuotedString(String::from( | ||
| "123:45.67", | ||
| )))), | ||
| value: Box::new(Expr::Value( |
There was a problem hiding this comment.
minor stuff we could use the Expr::value here too
There was a problem hiding this comment.
Yes! But I don't have the strength to fight ast-grep again 🙂
There was a problem hiding this comment.
I saw it in my editor and couldn't exist:
| "SELECT ", // line 1, column 1 | ||
| "'single', ", // line 1, column 7 | ||
| r#""double", "#, // line 1, column 14 | ||
| "'''triple-single''', ", // line 1, column 22 | ||
| r#""""triple-double""", "#, // line 1, column 33 | ||
| r#"'single\'escaped', "#, // line 1, column 43 | ||
| r#"'''triple-single\'escaped''', "#, // line 1, column 55 | ||
| r#"'''triple-single'unescaped''', "#, // line 1, column 68 | ||
| r#""double\"escaped", "#, // line 1, column 83 | ||
| r#""""triple-double\"escaped""", "#, // line 1, column 92 | ||
| r#""""triple-double"unescaped""", "#, // line 1, column 105 | ||
| r#""""triple-double'unescaped""", "#, // line 1, column 118 | ||
| r#"'''triple-single"unescaped'''"#, // line 1, column 131 |
There was a problem hiding this comment.
do the added comments here need cleanup or do they serve a purpose?
|
🎉 |
4 participants
@iffyio @alamb : I allowed edit by maintainers on this PR; help would be very welcome.
I implemented all the required changes in the library itself, but there are still massive (mostly repetitive and probably automatable) changes to make in the tests.
I would love if I could get some help on this, this should greatly decrease missing spans in parsed trees.