-
Notifications
You must be signed in to change notification settings - Fork 605
Add support of locations in AST #524
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Comments
There is some prior art / discussion Here are some PRs that attempt to do the same thing: Basically the challenge of this will feature in my mind are to add the feature without causing "too much" code churn for users of this crate or requiring a massive design / code review (unless some of the other maintainers like @Dandandan or @maxcountryman or @andygrove are able to help too). See notes on https://github.com/sqlparser-rs/sqlparser-rs#contributing |
Some more prior art: #189 I'd be very interested in a lossless AST. I wanted to use this library to collect comments from a SQL file but that use case isn't supported right now since comments are discarded. |
If it's still of interest, I was able to revive #288 and get it working w/ fewer merge conflicts + passing tests. Happy to send a PR if interesting. |
@ankrgyl would love to see a PR -- basically if adding support won't cause significant maintenance burden and a reasonable (but not pain free of course) upgrade path. cc @AugustoFKL |
Here's the PR: #710. I have some thoughts about how it may be less painful to upgrade against, which I'll write as comments in the PR. |
Proposal
I go through the sources of the parser and find out that there is no such thing as a location in AST
I have in mind something like this:
lexer layer
parser layer
Behind motivation
Much better error reporting
Proposal design highlights
Why
Option<Loc>
?We can allow to run the CLI in different modes, with emitting and not emitting locations
Plan of migration
Doesn't cover it right now ( but I have in mind high-level plan of gradual migration). Just want to discuss the idea first of all
@alamb what do you think? I can drive this migration if we discuss details and make a decision about it
The text was updated successfully, but these errors were encountered: