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 ------------------------------------------------------------------------------ Dive into the World of Parallel Programming! The Go Parallel Website, sponsored by Intel and developed in partnership with Slashdot Media, is your hub for all things parallel software development, from weekly thought leadership blogs to news, videos, case studies, tutorials and more. Take a look and join the conversation now. http://goparallel.sourceforge.net _______________________________________________ Xmlunit-general mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/xmlunit-general
