search

PRQL / prql

PRQL is a modern language for transforming data — a simple, powerful, pipelined SQL replacement
https://prql-lang.org
Apache License 2.0
9.65k stars 208 forks source link

refactor: Move `Token` next to `Expr` #4521

Closed max-sixty closed 1 month ago

max-sixty commented 1 month ago

Previously Token was in prqlc-parser, and Expr (one of them...) was in prqlc-ast. This means that we can't add a comments field containing a Token to Expr when working on the formatting.

I'm increasingly of the view that splitting out prqlc-ast may not have been the correct approach, particularly given the maturity of our library, and that a simpler approach would be to collapse some of these distinctions; particularly those which don't help split up the stages of the compiler.

(While this increases the size of prqlc-ast, it arguably reduces the split's significance, so I think a reasonable small step to the goal above, as well as solving an immediate issue...)

max-sixty commented 1 month ago

CC @m-span

vanillajonathan commented 1 month ago

It would be good to have docblock comments too.

aljazerzen commented 1 month ago

Docblocks are unrelated to this change.

max-sixty commented 1 month ago

Yeah, I'm not sure about prqlc-ast too. It probably help compile times and it is a reasonable segmentation of the project. If we wanted to segment the project in the first place :D

Yes, i can def see the logic for splitting things up.

My prior was that splitting by compiler stage (e.g. the prqlc-parser) is good, but that the prqlc-ast wasn't by compiler stage and so didn't make things simpler.

@m-span given your work was some of the inspiration for cleaning this up, I really don't want to cause any conflicts with #4470. Should I merge this and then resolve conflicts? Or do you think you're almost there with that and want to merge first?

m-span commented 1 month ago

I think these changes are a good idea. I'm almost there - since my changes are a bit larger, I'd like to get mine in first

m-span commented 1 month ago

Just merged #4470, so this is unblocked

max-sixty commented 1 month ago

OK great, merging this too now.

And I'm increasingly of the view that we should move the parts of prqlc-ast that are consumed by prqlc-parser into prqlc-parser. Modularity is great but we should be emphasizing the modularity through the compiler stages, and the additional crate doesn't pull its weight without that.

...which I think is the view you proposed recently @m-span ...