-
Notifications
You must be signed in to change notification settings - Fork 29
WIP: Pretty error printer for predicates. #39
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
Conversation
Thanks for the PR @Aaronepower. Unfortunately, I'm worried this is a non-starter as it's currently written: I am quite strongly against requiring that all I'd be much more receptive to this idea if there were a way to make it opt-in, i.e. not require |
@nastevens, Aaron and I had talked about this before he posted this. It is very rare that Do you have an example of a case that'd be a problem? If this doesn't work out, we could turn this into an additional trait but that will get kind of messy, like expanding |
@@ -21,7 +21,7 @@ pub struct AndPredicate<M1, M2, Item> | |||
where | |||
M1: Predicate<Item>, | |||
M2: Predicate<Item>, | |||
Item: ?Sized, | |||
Item: ?Sized + fmt::Debug, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making this a dedicated commit
If we keep this, please note in the commit message that you are breaking the API.
Personally, I recommend following conventional changelog, it makes it easier for me to notice everything when deciding on the new version / writing the changelog.
src/main.rs
Outdated
@@ -0,0 +1,7 @@ | |||
extern crate predicates; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this in the current commit?
src/core.rs
Outdated
/// Execute this `Predicate` against `variable`, returning the resulting | ||
/// boolean. | ||
fn eval(&self, variable: &Item) -> bool; | ||
|
||
/// TODO | ||
fn eval_to_string(&self, variable: &Item) -> String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this in the current commit?
src/core.rs
Outdated
|
||
/// TODO | ||
#[cfg(feature = "term-table")] | ||
fn tree_eval(&self, variable: &Item) -> String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a way to programmatically know the result of the evaluation, so a caller like assert_cmd can decide whether to panic or not.
@nastevens Thanks for your feedback. There are types that don't implement |
Background: This is an implementation for #7 (btw if you note that in commit message as To get an idea of what I'm talking about, here is an artificial failure in, what to me, is the most ideal test runner I've ever used
This shows that the test fails and shows what the subexpessions evaluate to in a tree structure for easy parsing by humans. My original plan for this trait method was to find counter examples (passing in a My thought is that the The downsides to this
Now let's see how well I remember my conversation with @Aaronepower from yesterday. He wanted to start with a simpler implementation. For example, instead of visually representing it as a tree, what if we just list each sub expression in series. As for For now, we have a vertical slice to iterate on the design to minimize churn. |
src/core.rs
Outdated
@@ -8,15 +8,60 @@ | |||
|
|||
use std::fmt; | |||
|
|||
struct TreeWriter<'a, Item: ?Sized + fmt::Debug + 'a> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dead code?
Speaking of, should we have a ExpressionWriter
trait that flatten
accepts as a reference to decouple the allocation policy from the Predicate
trait?
src/core.rs
Outdated
/// Execute this `Predicate` against `variable`, returning the resulting | ||
/// boolean. | ||
fn eval(&self, variable: &Item) -> bool; | ||
|
||
/// TODO | ||
fn flatten<'a, 'b>(&'a self, _vec: &'b mut Vec<&'a Predicate<Item>>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, what name should we use for this?
src/core.rs
Outdated
} | ||
|
||
/// TODO | ||
fn stringify(&self, _variable: &Item) -> String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, what name should we use for this?
The difference between this and Display is that this includes and evaluation of var
/ actual
.
src/ord.rs
Outdated
} | ||
|
||
impl<T> fmt::Display for EqPredicate<T> | ||
where | ||
T: fmt::Debug, | ||
{ | ||
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
write!(f, "var {} {:?}", self.op, self.constant) | ||
write!(f, "{} {:?}", self.op, self.constant) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is var removed?
I've added a commit that switches it from a table output to a tree output so that we can compare and contrast bwetween the two. |
I admit I am having a really hard time thinking of an example in my own usage where requiring That said, I'm coming around to the idea that this change is a minor step backwards (API is slightly more restrictive in what it accepts) for a major step forward (a human can actually get visibility into their |
Sorry I'm just now getting back to this. I've started to record in #7 the different lessons we requirements and issues we learned through this. I'll hopefully get back to this again soon with either more feedback or yet another idea for us to try. |
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.
Again, thanks @Aaronepower for your work on this. This helped a lot in uncovering requirements (recorded in #7) and helped pave the way for #60. Unless there is concern otherwise, I'm considering #60 to supersede this PR. |
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.
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.
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.
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.
Currently missing implements for a lot of the predicates currently only
AndPredicate
,OrdPredicate
, andEqPredicate
have correct impls.Table Output
Tree Output