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

feat: expose pretty printing helper for mast nodes #1441

Merged
merged 2 commits into from
Aug 10, 2024

Conversation

bitwalker
Copy link
Contributor

This PR exposes MastNode::to_pretty_print, which is too useful to keep internal to the crate, and without it, there is effectively no way to emit the textual MAST for specific nodes, such as in debug output. Strangely, Program itself implements PrettyPrint, but Library does not (and we would want to take advantage of the additional metadata in Library to PrettyPrint it differently anyway). The compiler use cases I have could probably be solved for by implementing pretty printers for Library and ProcedureInfo as MAST, with accompanying path/digest information, but I think there is value in exposing the pretty printer for MastNode directly anyway, since there may still be use cases where that is handy.

We should probably aim to make things like this public unless we have a good reason not to, because in many cases, if it was useful to us inside the crate, it will be useful for downstream users of the crate as well (especially the compiler).

I'm going to pin the compiler to the tip of this branch (and next, once merged) until we can make a release to crates.io that contains it, since it is used in debugging efforts at the moment.

@bitwalker bitwalker added the enhancement New feature or request label Aug 10, 2024
@bitwalker bitwalker self-assigned this Aug 10, 2024
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you!

Should I release miden-assembly and miden-core crates with these changes under v0.10.1?

@bitwalker
Copy link
Contributor Author

@bobbinth That'd be great!

One thing I also noticed btw, but you published crates for our thiserror and miette forks, but the dependencies in next are still referencing my git forks, we probably should just switch to using miden-thiserror, etc.

@bitwalker bitwalker merged commit 828557c into next Aug 10, 2024
9 checks passed
@bitwalker bitwalker deleted the bitwalker/expose-useful-helpers branch August 10, 2024 18:06
@bobbinth
Copy link
Contributor

I've published v0.10.2 crates which include these changes and also changes #1439 and #1442. #1442 technically has breaking changes against v0.10.0 but I think that's OK as we published v0.10.0 pretty recently.

Also, next is now in sync with the main.

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

Successfully merging this pull request may close these issues.

2 participants