alamb commented on PR #1435:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1435#issuecomment-2468945628
Sorry for my silence / inability to find time to review this PR in depth. I
am struggling with too many thing
TLDR I I think we should:
1. Get the tests passing on this PR and merge it in (if not all AST nodes
have spans initially, we can file tickets to add the information over the next
few releases)
2. Add some documentation explaining what is happening and how downstream
consumers need to change (I think basically by ignoring the newly added fields
@iffyio and I spoke about this a bit -- basically in my opinion adding
source locations to sqlparser-rs is one of the last major remaining features it
is missing.
However, it also has the potential to be very disruptive to downstream
consumers.
It seems to me the requirements for this change are:
1. Any changes required by users of this crate should be mechanical
3. We should clearly document what changes are needed on upgrade (ideally
with examples)
An example of a mechanical change might be adding
```rust
match node {
ast::Query {
field1,
field2,
location: _, // add a new line to ignored location
}
```
An example of a non mechanical change might be wrapping all AST nodes in
some other struct so the structure of the `match` no longer works
I looked at this PR and I think it basically satisfies 1 for me so I think
we should proceed. Would it be possible to write up something, perhaps in the
`README` that explains that we are adding new spans to the code and how we
will handle them?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
