Implement aggregation of error messages for different entries in maps

[kelebra]

This PR tries to extend Fact API to accommodate for requirements in #38. The implementation is somewhat rough on the edges but gets the main idea across as well as improves error messaging for maps.

We can iterate on this PR and come up with the handling of the corner cases and/or simplification. The rendering of the AST is not generic but is reasonable for now. We can definitely discuss this more in detail in this PR.

[kelebra]

Pinging @cocuh for review :)

[cocuh]

Sorry for late reply, and thank you for PR! I took vacation for weeks, and I came back today. Let me review the PR tomorrow. 🐱

[cocuh]

Thank you for your contribution! and sorry for taking time to review! 🐱

[kelebra]

Is there any possibility of creating two or more nested facts?

With the structure proposed in this PR - yes, definitely and that was the goal. Consider the proposal in #38:

"keys were mapped to an unexpected values": [
  "key1": {
    "expected" : "val1",
    "actual"      : "val2" 
  }
  ...
]

This is exactly the shape that you refer to in your "first": {"second": {"actual": "xxx", "expected": "yyy" }} only with the difference of first being mapped to a list.

This means generalization of nest is probably overengineering, as one nested should be sufficient.

Depends on how you look at it and the abstraction you are comfortable operating with. We established that grouping key-value comparison is a good idea and clarifies the output and the proposed structure achieves exactly that. There are more complicated cases of comparison (i.e BTreeMap or nested Map) that current library does not support and where the flexibility of proposed abstraction could be beneficial (i.e grouping out-of-order entries, nested diff etc).

It is one option to add a enum entry that doesn't need nest for example Fact::KeyedDiff {key: String, actual: String, expected: String}, which is close to MapFactDiff. I'm wondering if this is sufficient to fix the original bug or not.

Not really, we would need grouping and something that would encapsulate a single diff in the map. Most likely this means creation of the struct outside of the Fact (similar to the missing Map entries) and construct an instance of Fact::KeyValues that will render the diff structs as strings, something like this:

"keys were mapped to an unexpected values": [
  - "key1": "expected" : "val1", "actual" : "val2" 
  - ...
]

There are couple of problems with this approach:

1. The format of the message is actually controlled outside of the base module. This means that we spread the rendering logic between individual diffs and the engine.
2. Colorization in Better diff representation using bold / color #29 becomes more complicated since the structured representation now is located outside of the base module.
3. Handling cases of large values or keys becomes a matter of raw string manipulation.

Do you have any thoughts on this?

If we would like to keep implementation more based to the cases we have at hand we could combine our ideas and start by creating a Diff enum that encapsulates differences that engine can render:

enum Diff {
  ValueDiff { expected: T, actual: T },
  KeyValueDiff { key: T, diff: ValueDiff },
  KeyValue {key: T, value: T},
  Value {value: T}
  // we can also add `Extra` and `Missing` if we need more structure
}

And then we modify the Fact so that it supports Diff instead of raw strings and repurpose key as message that we render for the Diff or grouped diffs:

pub enum Fact {
    DiffFact { message: String, diff: Diff },
    DiffGroup { message: String, values: Vec<Diff> },
    Message { content: String }, // no need for Value
    Splitter,
}

wdyt? I think that might be a good path forward and allows for simpler implementation of colorization for diffs. You would just have to work on Diff rendering ignoring the Fact altogether.

[kelebra]

@cocuh thanks for the initial review, what do you think about the proposed approach?

[cocuh]

Hmmm, this is difficult discussion.

There are more complicated cases of comparison (i.e BTreeMap or nested Map) that current library does not support and where the flexibility of proposed abstraction could be beneficial (i.e grouping out-of-order entries, nested diff etc).

I don't think this flexiblity produces readable output. Each fact generation logic should summarize results for readability as much as possible based on the knowledge of the structure. For example, comparison of nested map, if inputs are ...

actual: {"parent_key": {"child_key": {"grandchild_key": "actual"}} }
expected: {"parent_key": {"child_key": {"grandchild_key": "expected"}} }

and nested output can be..

keys were mapped to an unexpected values
"parent_key":
    "child_key":
        "grandchild_key": 
            actual: "actual"
            expected: "expected"

but, I think next output is better, and extensible if having many diffs.

Following paths are different
parent_key.child_key.grandchild_key: "actual" -> "expcted"

or 

parent_key.child_key.grandchild_key:
    actual: "actual"
    expected: "expected"

Structured fact may be useful when writing library code, but I think it will reuslt in damaging user experience because of allowing compricated error message to users. (and also it makes library more complicated)

---

About your proposal, I don't think introducing type parameter to Fact structure is a good option, because we should put all object specigic logic under XXXAssertion.

If we introduce the type variable to Fact, generating message in AssertionResult gets able to change the logic to format the value of T like using some custom trait. Theoretically, impelmentation of the custom trait can be specialized for each T to make it more descriptive, but this means we need implement two parts; diffing part (happing in XXXAssertion) and formatting part (new custom trait) including how to propagate diff info between two to avoid inconsistency. From UX perspective, desired formatting can be different based on what is asserting / context, delegating formatting to AssertionResult won't be good result. Current design (generating fact as strings in XXXAssertion utilizing data used in diffing) is more straightforward, easier and flexible.

---

I agree on your points (1,2,3) on current design. About the first point, we should minimize the copy-paste though, scattering can be interpretted as customizable for each type. The first two items can be resolved by introducing util functions as intermediate layer; for example you introduced in diff.rs. About the third point, it is ineviable at some point because the final output of this library is stdout.

I exlored options for colorization and noticed this is much more complicated than my expecatation. Let's ignore it in this issue.

Thanks, 🐱

[kelebra]

Thanks for the feedback @cocuh and sorry for the late update! I implemented the message consolidation according to your suggestion, PTAL.

Apologies if I made the conversation difficult, that was not my intention. I wanted to propose the approach that I think is better which might be somewhat complicated for the current stage of the library development. Thanks for your patience and detailed opinion.

[cocuh]

Thank you for updating PR! LGTM for the change, and I appreciate your suggestion and discussion happened in this thread as it helped me to come up with better design on the colorization.