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

Creating a Tree is expensive #3

Open
epage opened this issue Jul 21, 2018 · 4 comments
Open

Creating a Tree is expensive #3

epage opened this issue Jul 21, 2018 · 4 comments

Comments

@epage
Copy link
Contributor

epage commented Jul 21, 2018

I am using this crate to show what parts of an assertion failed and why (see assert-rs/predicates-rs#7).

For simple nodes, it seems like the current API is great. Unfortunately, I have heterogeneous nodes for my expression tree. so with treelines current API, I have to copy this into a treeline specific tree and convert each of my nodes into a Box<Display> to conform to the API, making a lot of unnecessary work.

@epage
Copy link
Contributor Author

epage commented Jul 21, 2018

I wonder if we could create a Tree trait, maybe something like:

pub trait Tree {
    fn root(&self) -> &Display;
    fn leaves(&self) -> Box<Iterator<Item=&Tree>>;

    fn display(&self) -> DisplayTree;
}

pub struct DisplayTree(...);

impl Display for DisplayTree {
...
}

This would allow heterogeneous nodes, reducing unnecessary Box::new(item.to_string()), instead allowing items existing Display to be directly leverage.

The existing Tree implementation could still exist for rapid prototyping and those that don't have an complex trees.

epage added a commit to epage/predicates-rs that referenced this issue Jul 21, 2018
Finnaly, Fixes assert-rs#7.

Inspired by the work in assert-rs#39.

Created softprops/treeline#3 for trying to find ways to make this more
efficient.
epage added a commit to epage/predicates-rs that referenced this issue Jul 21, 2018
Finnaly, Fixes assert-rs#7.

Inspired by the work in assert-rs#39.

Created softprops/treeline#3 for trying to find ways to make this more
efficient.
@softprops
Copy link
Owner

In like your idea. If you have some time I'd be open to a PR. Otherwise I can take a look

@epage
Copy link
Contributor Author

epage commented Jul 21, 2018

Unfortunately, this is one of the lower items on my priority list (using various tricks to avoid treeline being in my public API, allowing me to have it be improved later).

If curious, you can see my use of treeline in assert-rs/predicates-rs#60. predicates is forming the basis of a flexible assertion system I'm working on, with assert_fs and assert_cmd being my priority clients.

@epage
Copy link
Contributor Author

epage commented Jul 21, 2018

I have mixed feelings about the display method vs impl<T> Display for T where T: Tree

For my use case, I'm probably going to do this

pub trait CasseTreeDisplayExt {
    fn tree(&self) -> CaseTree;
}

pub CaseTree(Case);

impl Tree for CaseTree

and ideally, CaseTree is Display. The reason for the newtype is my tree code will live in a separate crate from Case for minimizing semver breaks for my core logic.

epage added a commit to epage/predicates-rs that referenced this issue Jul 28, 2018
Finnaly, Fixes assert-rs#7.

Inspired by the work in assert-rs#39.

Created softprops/treeline#3 for trying to find ways to make this more
efficient.
epage added a commit to epage/predicates-rs that referenced this issue Jul 28, 2018
Finnaly, Fixes assert-rs#7.

Inspired by the work in assert-rs#39.

Created softprops/treeline#3 for trying to find ways to make this more
efficient.
epage added a commit to epage/predicates-rs that referenced this issue Jul 28, 2018
Finnaly, Fixes assert-rs#7.

Inspired by the work in assert-rs#39.

Created softprops/treeline#3 for trying to find ways to make this more
efficient.
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

2 participants