Make Assert and hierarchy public visible to enable better custom extensions

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Create class MyCustomTypeAssert
2. Let class MyCustomTypeAssert extend (Generic-) Assert 
3. Enjoy the compilation error "... the type org.fest.assertions.Assert is
not visible"

What is the expected output? What do you see instead?
The current extension mechanism is un-handy.  For the most custom types one
needs exactly the methods provided by Assert or GenericAssert. Therefor we
would like to be able to use them.

We think that you did make the classes package protected because you want
the clients to use the Assertions class which provides static factory
methods for all known Assert classes. 

The advantage of this way is the single static import of Assertions.assertThat.

The disadvantages are:
1.) Assertions is a dependency magnet - the class depends on every Assert
class within the framework. 
2.) It violates the "open-closed-principle". New Assert classes cannot be
introduced into the framework without changing the Assertions class.
3.) Custom out-of-framwork extensions are not possible.

For the static factory methods we suggest to let each Assert class
implement the method assertThat(..) which returns the custom Assert class
instance. 
The Assertions class would not be necessary any more because one could just
use the specific methods. 
The only little drawback (if you don't use an IDE :-) are the multiple
static imports.

Example: Take a custom typ:

public class Money
{
  .... do what you want ..
}

Than we would like to write a custom Assert class

public class MoneyAssert extends GenericAssert<Money>
{
  // provide any additional checks not supported by GenericAssert

  public static MoneyAssert assertThat(Money actual)
  {
    return new MoneyAssert(actual);
  }
}

The client usages looks like :

static import your.name.MoneyAssert.assertThat;
...
assertThat(euro("5")).isNotNull();

We are looking forward to any feedback or comments.

Regards,

Jacob Fahrenkrug and Timmo Gierke

Original issue reported on code.google.com by timmo.gi...@gmail.com on 23 Dec 2008 at 4:14

[GoogleCodeExporter]

Hi Jacob and Timmo,

I like the approach for extension you are proposing. I have a question though. 
We
currently provide an extension mechanism (
http://fest.easytesting.org/assert/wiki/pmwiki.php?n=FEST-Assert.CustomAssertion
s .)
Is this mechanism missing some functionality you need, or is there any way we 
can
improve it?

I'm not opposing making APIs more visible, I just want to make sure that we 
currently
have is useful :)

Side note: if we let each assertion class implement 'assertThat' as a static 
method,
we will not be able to use static imports, the compiler will complain if we try 
to
statically import methods with the same name but from different classes (AFAIK, 
I
could be wrong...I need to confirm this.)

Many thanks,
-Alex

Original comment by Alex.Rui...@gmail.com on 23 Dec 2008 at 4:31

• Added labels: Milestone-Release1.1, Module-fest-assert, OpSys-All, Priority-High, Type-Enhancement

[GoogleCodeExporter]

Hi Alex,

thanks for your very quick response and positive feedback!

Yes we saw and tried the existing extension mechanism. 

Short comings from the clients perspective: 
Why do I first have to instantiate the custom assert (passing in parameters to 
the
constructor) and afterwards have to call assertThat? 
The extension behave different than the "real" framework code - something we
definitively not want (two things to know!).  

Short comings from the implementers point of view:
One have to implement all "standard" checks over and over again. The extension
mechanism forces me to again define and implement isNull, isNotNull, isEqualTo, 
... 
And even worse: The static "helper" methods in class Fail (and hierarchy) like
fail(...) or failIfNotSame(..) are package protected. Result: The implementer 
has to
code the error message formating itself.

To sum up: The extension mechanism is of help but we could do better :-)

On you side note:
We ouselfs have not been sure if this is valid java syntax but it is. You can 
have
multiple static method imports with same name but different arguments in one 
class.
It behaves like overloaded static methods in one class.

(To be honest: Intellij IDEA (our current IDE) has a minor bug with this 
feature. If
you have two import lines below each other IDEA magically removes one. Put it 
blank
line in between one and everything is ok :-) 

Regard,

Timmo Gierke

Original comment by timmo.gi...@gmail.com on 23 Dec 2008 at 5:09

[GoogleCodeExporter]

Hi,

please find a quick UML class diagram attached. 

Timmo

P.s.: Mary Christmas!

Original comment by timmo.gi...@gmail.com on 23 Dec 2008 at 5:47

Attachments:

• IMG_0805.jpg

[GoogleCodeExporter]

Hi Timmo,

Thanks for the Christmas wishes! Merry Christmas to you too! :)

I like the diagram! Very helpful!

I agree, GenericAssert should be visible. I'll address this problem shortly. 

I also like the idea of having each assertion implement its own "insertThat". 
Making
this change will break backwards compatibility (not a bad thing, IMHO.) I'd 
need to
release fest-assert as 2.0 when this change is implemented. Also, I'd like to
experiment a little bit before committing to make this change.

I'd prefer to make the first change first, release it as 1.1 and then implement 
the
second change...and release again (2.0). What do you think? :)

Thanks!
-Alex

Original comment by Alex.Rui...@gmail.com on 23 Dec 2008 at 7:56

[GoogleCodeExporter]

Looking at releases is good. 

The 2.0 release without the Assertions class sounds possible if there is a 1.1 
or
even 1.2 release in between!
I would suggest to make the Assertions class deprecated for the 1.1 or 1.2 
release so
everybody knows early: Remove the usages before the next major release!

Additional for the 1.1 release I would look forward to have the assertThat 
methods in
each leaf of the Assert hierarchy. 
This does NOT break backward compability. As long as you leave the public API 
of the
"Assertions" class untouched we have no problem. 
To avoid code duplication the Assertions.assertThat(*) methods should delegate 
to the
assertThat methods in the specific classes. 

It would be great to see the Assert class open for extension in the 1.1 release.
But to be fair, please let me remember you: 
Developing an API for client usage is hard work. 
Developing an API for sub classing is much harder!

Thank you for your engagement!

  Timmo  

Original comment by timmo.gi...@gmail.com on 23 Dec 2008 at 10:32

[GoogleCodeExporter]

Changes made in r2238 ( http://code.google.com/p/fest/source/detail?r=2238 ) 
Please
feel free to review them :)

I decided not to change the contract of the Assertions class for now. I agree 
that
adding the 'assertThat' method to each assertion class will not break code
compatibility. I'd like to see how this would work with static imports, which 
is a
big deal for me.

Many thanks for filing this issue.

Thanks,
-Alex

Original comment by Alex.Rui...@gmail.com on 22 Jan 2009 at 6:02

• Changed state: Fixed

[GoogleCodeExporter]

Hi Alex,

I noticed you did a number of releases of other FEST subprojects, so maybe you
already are in a mood of releasing software :-)

We're a bit eager to see a FEST-assert release, since the changes made in r2238 
are
essential before we can introduce FEST-Assert into our project. Of course, we 
would
like to start using FEST assert asap. Can you give us a tiny update regarding 
the
release schedule for FEST-Assert 1.1? When do you plan to release it?

TIA

Ansgar

Original comment by ansgar.k...@googlemail.com on 26 Feb 2009 at 8:31

[GoogleCodeExporter]

Hi Ansgar,

I think Ted Young is working on some features to add for the next release. I 
would
feel really bad releasing and not including his work. I'm thinking about having 
a
SNAPSHOT release (1.1-SNAPSHOT.) And then make the full 1.1 release once Ted is
ready. Would that be helpful?

As always, thanks a lot for your interest in the project and for helping us 
improve it :)

Regards,
-Alex

Original comment by Alex.Rui...@gmail.com on 26 Feb 2009 at 7:24

[GoogleCodeExporter]

Hi Alex,

an intermediate release would be very much appreciated. However, as far as I 
know,
maven does not support releasing SNAPSHOTs, since SNAPSHOT in maven terms is by
definition something not fixed, still in progress. A release should be a _fixed_
revision of the codebase. Just give the release a different name and everything
should be fine (e. g. 1.1-MILESTONE1).

Kind regards

Ansgar

Original comment by ansgar.k...@googlemail.com on 27 Feb 2009 at 9:17

[GoogleCodeExporter]

Hi Ansgar,

Sorry for the lack of progress. I'll be publishing a MILESTONE1 version in our 
Maven
Repo between tonight and tomorrow.

Best regards,
-Alex

Original comment by Alex.Rui...@gmail.com on 4 Mar 2009 at 6:55

[GoogleCodeExporter]

Hi Alex,

any progress regarding the milestone release? Not that I want to push, would 
just be
nice if you could give me a tiny update.

Kind regards

Ansgar

Original comment by ansgar.k...@googlemail.com on 9 Mar 2009 at 7:44

[GoogleCodeExporter]

Hi Ansgar,

Sorry for the late response. Moving FEST to CodeHaus took most of my time. I 
just
uploaded fest-assert-1.1a1 with the changes you proposed. You can find this 
release
at 
http://fest.googlecode.com/svn/trunk/fest/m2/repository/fest/fest-assert/1.1a1/

Thanks a lot for your patience! :)

Best regards,
-Alex

Original comment by Alex.Rui...@gmail.com on 9 Mar 2009 at 8:09

[GoogleCodeExporter]

Hi Alex,

thanks a lot! :-) Be assured that your FEST Assertions will find their way into 
our
projects during the next few days.

Kind regards

Ansgar 

Original comment by ansgar.k...@googlemail.com on 9 Mar 2009 at 9:50