Refactor "overridingErrorMessage" and "usingComparator" to keep methods in a chain as single units #115

Open
alexruiz opened this Issue Sep 18, 2012 · 6 comments

Projects

None yet

2 participants

@alexruiz
Owner

The methods in the FEST-Assert API must be considered as individual units. As such, a method in the chain should not partially affect another method in the chain. Readability is lost when this happens.

Please read for a better explanation: http://alexruiz.developerblogs.com/?p=2568

@alexruiz alexruiz was assigned Sep 18, 2012
@joel-costigliola
Collaborator

Nice post, very clear !

I agree for overridingErrorMessage because it was meant to affect only one assertion but not for usingComparator because it applies to all chained assertions occuring after the call to usingComparator as shown in this example :

assertThat("Frodo").usingComparator(caseInsensitiveStringComparator).startsWith("fr").endsWith("ODO");

Note that if you start a new assertions chain, the comparator used falls back to the default one.

@alexruiz
Owner

Thanks Joel!

Nice use case. I agree with you. What makes me uncomfortable is having a method in the chain that is not an assertion, that breaks the style of the fluent interface. I propose the following:

assertThat("Frodo", usingComparator(caseInsensitiveStringComparator)).startsWith("fr").endsWith("ODO");

There it is clear (or at least more clear) that the comparator is being used for all assertions in the chain. There are 2 more things that I quite don't like:

  • Using comparators for primitives
  • Resetting to the default comparison mechanism in the chain

We'll address those in a separate bug :)

@joel-costigliola
Collaborator

I see your point on chaining only assertions, if we follow this logic then we will have to refactor describedAs methods.

If we follow your proposal, we should check that it is still IDE friendly and typesafe, in my example you must provide a Comparator<String> because you are making String assertions.

For the primitives comparator, I'm waiting for your comments but I can see some use case, for example applying a tolerance for equals checks (offset as we named it), let's open that discussion :)

On resetting to the default comparison, I did it to be more feature complete but I admit there is not much of a use case there, so removing it would a good thing to simplify our API.

@alexruiz
Owner

Ahh...you are right. "as" and "describedAs" fall in the same category.

I strongly think the 2.x release is our chance to provide a really nice API. Let's not rush things, and let's experiment with different ideas. Luckily to us, you have been releasing frequently, so users have something to use in the meantime :)

It would be nice to release 2.x (final) soon, but I really don't want to later on start a 3.x one.

@joel-costigliola
Collaborator

By the way, to simplify API, what about removing "as" and "describedAs" with Description parameter and keep only the String parameter version ?
The reason is that I don't see any use case for a description other than a text.

@alexruiz
Owner

There is a use case: lazy-loaded descriptions. This feature was requested by a user years ago, and we actually found it pretty useful in FEST-Swing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment