Skip to content

Better node display - #137

Open
benwr wants to merge 8 commits into
ISibboI:mainfrom
benwr:better_node_display_rebased
Open

Better node display#137
benwr wants to merge 8 commits into
ISibboI:mainfrom
benwr:better_node_display_rebased

Conversation

@benwr

@benwr benwr commented Jun 3, 2023

Copy link
Copy Markdown
Contributor

Change the Display instance for Node. Previously it printed a simple pre-order traversal, which was sometimes ambiguous because e.g. for nested n-ary functions it wasn't clear which things were arguments.

Now, this should produce a visual that is (a) unique, and (b) can be parsed back into an equivalent Node.

@ISibboI ISibboI left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thank you a lot, this looks great! Can you add some tests with some simple and more complicated expressions whose Display output parses into the same operator tree?

Comment thread src/tree/display.rs
@benwr

benwr commented Jun 4, 2023

Copy link
Copy Markdown
Contributor Author

How would you feel about adding a dev dependency on quickcheck? I might want to randomly generate operator trees and check that they round-trip to the same trees.

@ISibboI

ISibboI commented Jun 8, 2023

Copy link
Copy Markdown
Owner

I am totally fine with new dev dependencies.

@ISibboI
ISibboI force-pushed the better_node_display_rebased branch from b56a199 to 3593b8f Compare June 17, 2023 09:53
ISibboI
ISibboI previously approved these changes Jun 27, 2023
@ISibboI

ISibboI commented Jun 27, 2023

Copy link
Copy Markdown
Owner

It seems like propagating the precision does not work. We can either just not support floating point precision, or we fix that somehow.

@ISibboI
ISibboI force-pushed the main branch 6 times, most recently from e4a8571 to 6608b16 Compare October 11, 2024 15:01
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.

2 participants