Re: [Xmlunit-general] xmlunit.org

[Stefan B. <bo...@ap...>]

On 2015-01-06, Harald Brabenetz wrote:

>> 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".

The best workflow is to create one branch per feature you want to
contribute and create a pull request from that.  After review you'd
rebase the branch to a single commit (squash it) and I'll merge that
commit.

Of course this is cumbersome if you are working on several
interdependent things at once and it may not always be possible, so
we'll have to adjust.

I prefer to have you make changes over me picking commits and modifying
them as it will be easier for you to rebase your branches once the PR is
merged this way.  Also it will show you as the contributor more clearly.

I hope this is not feeling like I was playing "fetch me a rock" with
you, I very much appreciate your work and your patience.  Thanks!

>> 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.

The idea in Java8 code

static Map<Class<?>, Function<?, Builder>>  KNOWN_BUILDERS ...
static {
    KNOWN_BUILDER.put(URI.class, u -> Input.fromUri((Uri) u));
}

Builder from(Object source) {
    Function<?, Builder> f = KNOWN_BUILDERS.get(source.getClass());
    return f == null ? f.apply(source) : fromJaxb(source);
}

of course the target is Java 1.6 so the "functions" will be anonymous
inner classes and the static block will look ugly.

>> 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/Builderby e.g.: Input.fromUri("...").

You could try to be smart and guess whether the String is an XML
document (starts with a "<") or looks like an URI.  I'm not sure it
would be worth the effort, though.

> 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.

Likely depends on how much you do with the "Source" in the rest of your
tests, but I get your point.

>> 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.

We'd better cache the reflection result in order to save runtime cost,
though.  Yes, that would be alternative to an optional 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 may want to look at the "SIMILAR" differences anyway?  Or I may want
to get all differences like 1.x's DetailedDiff.

>> * I'd likely remove Diff#getFirstDifference completely

> But how? I need the first Difference to create a nice
> Exception-Message (or in Diff.toString())

differences.get(0) in toString?  It just doesn't need to be public.

> But I think that one DifferenceEvaluator is not enough.
> At least DifferenceEvaluators.Default should always be part of,
> otherweise there is no ComparisonResult.SIMILAR.

IMHO this is something the user of the API should take care of.  If they
want to have DEFAULT's behavior then they should include it explicitly.

> But then I need a method like DifferenceEvaluators.inSequence(...)
> instead of DifferenceEvaluators.first(...).

first() picks the first that changes the outcome, what would be the
result of inSequence()?

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

The first one hints at the missing feature of ignoring parts completely.
I don't think DifferenceEvaluator is the best place for this.  The
second one sounds like a third jar of xmlunit-library rather than -core?

>> * 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.

true

>> 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 :)

understood.

>  - 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.

Since I'll merge your JAXB branch as soon as you open a pull request you
better create a new branch for the rest ;-)

>  - Input.fromUnknown(): I can rename or remove it. Let me know.

I'm not completely against including it.  Others?

>  - 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

Thanks.

> 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.

I'm not sure about my time constraints either, this is just your normal
"I do work I find time for it and want to do so" pattern of open source.
Don't feel obligated to do anything, this is supposed to be fun.

As next steps I want to add a bit of javadocs and publish them from the
xmlunit.org website and after that start migrating things (i.e. the Java
part) to Maven.

Stefan