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

Replace libdash pretty printer with shasta? #33

Open
BolunThompson opened this issue Dec 30, 2024 · 6 comments
Open

Replace libdash pretty printer with shasta? #33

BolunThompson opened this issue Dec 30, 2024 · 6 comments

Comments

@BolunThompson
Copy link
Contributor

BolunThompson commented Dec 30, 2024

Would it be possible to merge the libdash and shasta pretty printers? There have been multiple PRs (#31, #32) that have had to be duplicated across shasta, libdash ocaml and libdash python.

The necessary way of doing this would be to remove the libdash python pretty printer, and instead require a pass through shasta, since shasta already needs to be able to print its typed ast.

Referenced here.

@BolunThompson
Copy link
Contributor Author

There’s also #25, which proposes that libdash should return shasta asts, which seems like a sound approach.

@mgree
Copy link
Collaborator

mgree commented Jan 3, 2025

The only blocker here is that I would want to move https://github.com/mgree/smoosh forward at the same time. I'm not sure when I'll have time to push on all of that, but it should be a fast enough change.

@angelhof
Copy link
Member

angelhof commented Jan 5, 2025

Hmm, not sure I understand why moving smoosh forward is a blocker. Can't it keep using the old libdash version?

@mgree
Copy link
Collaborator

mgree commented Jan 10, 2025

My concern is that we'd discover some incompatibility after doing just one part, and then need to do extra work. Not the end of the world if so.

@angelhof
Copy link
Member

Oh I see, I am not sure how an actual incompatibility could be introduced though because we will just import a different library to do the exact same pretty printing process. Nothing else will change based on my understanding!

@mgree
Copy link
Collaborator

mgree commented Jan 16, 2025

Ah---you're absolutely right, since smoosh is using its own OCaml pretty printer. Totally fine to go ahead. (It's #25 that could be a bigger issue.)

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

No branches or pull requests

3 participants