Re: [Xmlunit-general] xmlunit.org

[Harald B. <bra...@gm...>]

Hi,

thanks for the quick review.

> I understand. In general I'd prefer finer grained PRs (even squashed to
> a single commit) as they are easier to review but I understand this is a
> problem to ask for them after the fact.
> Is all of the JAXB builder in the single commit
> https://github.com/brabenetz/xmlunit/commit/54923ce18be30aa340792a1e53dd420ff5e44a8a

Not everything. e.g.: JavaDoc and Input.fromUnknown returns a Source instead of a Builder.
I will create a separate branch "XmlUnit_2.x_ready_to_push".

> It is also inside the patch that introduced the JAXBBuilder. I
> understand the idea, maybe just "from" would be a better name - and I'd
> rather use a Map that a chain of instanceofs.
I'm not sure if "instanceof" can be handled by a Map.

> One problem I see is how to interpret a String arg - you've decided to
> use fromMemory but fromFile or fromUri would also be legitimate choices.
It's only a simple short-cut. Certainly, you can always use a Source/Builder by e.g.: Input.fromUri("...").

> I guess I need to review the DiffBuilder stuff to understand how you
> intend to use it. Usually people will know what builder method they
> want to invoke :-)
Sure I know the input, but it makes my tests ugly :)
I would more like to write:
	DiffBuilder.compare(control).withTest(test);
then
	DiffBuilder.compare(Input.fromSomething(control)).withTest(Input.fromSomething(test));

I think that was the only indention for Input.fromUnknown(...) :)
And there are no disadvantages. You can always use a Source or Input.Builder as input.

> The IDE visualization is certainly nice. An approach that might work is
> having an optional dependency and a fallback solution if
> ComparisonFailure is not there (like not throwing that exception and
> return false from matches).
I could create the ComparisonFailure by reflections only if it exists.
Then, not even the build-script to compile main/java-matcher will need the dependency.


> * Comparison is not good enough to capture a difference as the
> ComparisonResult is missing. So Diff should use a (new) class
> containing Comparison and outcome.
Yep, e.g. a "Difference.java" with Comparison and ComparisonResult as members.
My second thought was: ComparisonResult will be per default CRITICAL (required for break up after first difference)

> * I hadn't thought of ComparisonFormatter but your IDE example is an
> intriguing one. My choice would be to have a toString method with a
> ComparisonFormatter argument rather than an instance variable. The
> no-arg toString would always use the default fomatter and
> ComparisonMatcher could pass in a different one when needed.
looks great

> * nit-pick I'd use Iterable rather than List as return value
No problem.

> * I'd likely remove Diff#getFirstDifference completely
But how? I need the first Difference to create a nice Exception-Message (or in Diff.toString())

> * there can only be one DifferenceEvaluator, so the var-args versions
> are not needed - we may want to have a var-args
> withDifferenceListeners in DiffBuilder
withDifferenceListeners(): I forgot about that.

I have not tested withDifferenceEvaluator() well enough.
But I think that one DifferenceEvaluator is not enough.
At least DifferenceEvaluators.Default should always be part of, otherweise there is no ComparisonResult.SIMILAR.
But then I need a method like DifferenceEvaluators.inSequence(...) instead of DifferenceEvaluators.first(...).

I also think about a general SimpleIgnoreNodesDifferenceEvaluator (ignore some nodes)
or a SimpleBigDecimalNodesDifferenceEvaluator (which evaluates "1" similar "1.00") implementations.

> * DiffBuilder may want an option to short-cut the comparison as soon as
> the outcome is clear. I.e. return CRITICAL from the
> DifferenceEvaluator as soon as a DIFFERNT (or potentially a SIMILAR)
> result has been received.
Like DifferenceEvaluators.stopWhenDifferent(...) but more like:
DifferenceEvaluators.stopAt(ComparisonResult)
=> DiffBuilder.allDifferences(): return all differences (like now). Default is DifferenceEvaluators.stopAt(...) depending on checkFirstSimilar() or checkForIdentical()

> * my design would have ComparisonMatcher be a Matcher<Diff> and force
> people to create the Diff using DiffBuilder
> Diff diffResult = DiffBuilder...
> assertThat(diffResult, DiffMatcher.isSimilar());
> that way we didn't have to duplicate ignoreWhitespace and friends in
> two or three builders.
> Maybe have a DiffMatcher and a ComparisonMatcher as separate classes
This feels not right: In this case the Matcher doesn't make the matching.
A normal Matcher always contains the control Object to matching with.
I'm not sure if "CompareMatcher extends DiffBuilder" makes sense only to avoid the delegate methods,
but it could be a possibility.


> with ComparisonMatcher being Matcher<Source>?
> This would also reduce the need for Input.fromUnknown.
Yes, would be no Problem, but it would made my tests a little bit uglier :)

> * DiffBuilder should be in the builder package
Thanks.

---------------------
Summary
---------------------
 - I will crate a branch "XmlUnit_2.x_ready_to_push" with only the Input.fromJaxb(...). Later I will copy my other changes into this branch.
 - Input.fromUnknown(): I can rename or remove it. Let me know.
 - Create the ComparisonFailure by reflections if it exists (remove junit-dependency).
 - Create "Difference.java" with Comparison and ComparisonResult as members.
 - Create Diff.toString(ComparisonFormatter);
 - Diff.getDifferences() returns Iterator<Difference>
 - Add DiffBuilder.withDifferenceListeners(DifferenceListener...);
 - DiffBuilder.allDifferences(): default is DifferenceEvaluators.stopAt(...) depending on checkFirstSimilar() or checkForIdentical()
 - Move DiffBuilder into builder package

But it could take a while. From now on, I have only time on weekends.
The most of it I can do at the upcoming weekend.

Cheers
Harald