Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reorganize the Parser module #1581

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

davisp
Copy link
Member

@davisp davisp commented Dec 7, 2024

I mostly did this as an exercise to get a general feel of how the Parser implementation is organized. The basics here are that for every top level keyword in Parser::parse_statement I created a new module and moved the corresponding function to that module. Then I spent a few hours checking find references and any method that was in a single new module got moved there.

Towards the end I started making some arbitary decisions on where functions referenced from multiple modules lived. Some of these seemed obvious, while some were certainly arbitrary.

Most of the motivation here was that working on a 13,000 line file was causing my editor to be very not happy. After this, the largest module is now src/parser/select.rs which clocks in at 2142 lines.

I should note, that the only visible changes are hopefully a few functions that had visibility flipped from private to public because I forgot about pub(crate) when I first started. Other than that, this is purely copy/paste moving of code to new module files.

I mostly did this as an exercise to get a general feel of how the Parser
implementation is organized. The basics here are that for every top
level keyword in Parser::parse_statement I created a new module and
moved the corresponding function to that module. Then I spent a few
hours checking `find references` and any method that was in a single new
module got moved there.

Towards the end I started making some arbitary decisions on where
functions referenced from multiple modules lived. Some of these seemed
obvious, while some were certainly arbitrary.

Most of the motivation here was that working on a 13,000 line file was
causing my editor to be very not happy. After this happy, the largest
module is now src/parser/select.rs which clocks in at 2142 lines.

I should note, that the only visible changes are hopefully a few
functions that had visibility flipped from private to public because I
forgot about pub(crate) when I first started. Other than that, this is
purely copy/paste moving of code to new module files.
@davisp
Copy link
Member Author

davisp commented Dec 7, 2024

I originally had intentions on spending today trying to see if I couldn't figure out how to help move #1561 forward, but my usual attempt at commenting out the non-clone API made my editor choke on rendering the thousands of error diagnostics that caused. So I figured I'd try splitting up the 13k LoC parser.rs module. This is PR is the result of that.

I have no illusions that folks will want to merge this directly, but I figured it was a useful enough exercise that I'd show my work in case there's any desire to start doing something of this nature piecemeal with an eye towards having people that actually know this code base help refine some of my rather arbitrary decisions on the reorganization.

I managed to learn a bit about split impl blocks in the process so it was at least useful at that level. If the consensus is "Ehhhh, lets not" I'm totes fine with that being the outcome of this work and we can all collectively pretend like it never happend.

@davisp davisp force-pushed the pd/experiment/reorganizing branch from e0934e7 to 8d83ccf Compare December 8, 2024 00:39
You can't propose a 26k line diff when CI fails.
@davisp davisp force-pushed the pd/experiment/reorganizing branch from 8d83ccf to d1c90b2 Compare December 8, 2024 00:41
@iffyio
Copy link
Contributor

iffyio commented Dec 8, 2024

Thanks for drafting this @davisp! It does sound reasonable to me, I imagine this could help editors as you mention.
cc @alamb for thoughts if something like this could be desirable

@alamb
Copy link
Contributor

alamb commented Dec 9, 2024

Thanks for drafting this @davisp! It does sound reasonable to me, I imagine this could help editors as you mention. cc @alamb for thoughts if something like this could be desirable

I think breaking the code up into smaller modules sounds like a great idea. Thank you @davisp

I have no illusions that folks will want to merge this directly, but I figured it was a useful enough exercise that I'd show my work in case there's any desire to start doing something of this nature piecemeal with an eye towards having people that actually know this code base help refine some of my rather arbitrary decisions on the reorganization.

Indeed, I think doing this refactoring as a series of PRs would be great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants