Re: [Clirr-devel] Message framework

[Simon K. <si...@ec...>]

Hi Lars,

I'm just about to commit another batch of Message-related patches.
This email explains what they're about.

On Wed, 2004-06-30 at 11:27, Simon Kitching wrote:
> On Wed, 2004-06-30 at 08:47, Lars K=FChne wrote:
> >=20
> >     * I would probably have kept all Message instances in the Message
> >       class itself (typesafe enum idiom), not in the MethodSetCheck. =
The
> >       event package will be part of clirr's public API (for IDE plugi=
ns)
> >       and I think we don't want to have a public constructor for
> >       Messages. Typesafe enum also provides a central point of refere=
nce
> >       for all possible changes Clirr can find.
>=20
> By putting the Message objects on the check classes, I was trying to
> avoid "class version churn". With all the possible messages declared
> "enum-style" on the central Message class, whenever any test is added t=
o
> any check, the Message class needs to gets updated. With the design I
> committed, that doesn't need to happen.
>=20
> However there are other points of "churn" anyway: the .properties file
> and the "message description" html file. So one more is probably not a
> significant problem.
>=20
> So I'm happy to move the Message members from MethodSetCheck to the
> Message class itself. There are definite benefits to doing this
> (including being able to make the constructor private as you said).

I had a go at moving all the Message object declarations into the
Message class (enum-style) but it just didn't feel right. In the end I
felt my original approach of having the Message objects declared on the
check classes was better. Here's my reasons for this:

(a) As mentioned earlier, having all the Message declarations on the
Message class causes the Message.java class to be very frequently
changed in CVS. This just isn't tidy.

(b) Message objects really aren't ENUM constants. They are only ever
referred to "by name" from the point where the ApiDifference is created
and fired. So I don't feel the "enum" design pattern really applies
here.

(c) Having the Message class "know" about all the possible messages that
can be generated really is an un-necessary coupling. Yes, this is=20
probably only theoretical but I feel it isn't tidy.

(d) With all Message objects declared as members of the Message class,
it isn't possible for users to add any custom checks, or subclass
existing checks to add extra tests that can generate new messages. Note
that the current message framework doesn't currently make this simple
either, but it is easier to implement this in future if we don't expect
all Message objects to be members of the Message class.

(e) The current design allows the Message objects to be private members
of the check classes. Moving them to the Message class means they would
have to be public, just so they can then be referenced from exactly one
place in the code: the line in the check class where the message is
reported. Not only is this slightly untidy, but checkstyle requires each
public member to have a separate javadoc tag :-(

>=20
>=20
> >     * For testing I think it would be better if the translation outpu=
t
> >       language would be controlled by the DiffListener (add a Locale
> >       param do ApiDifference.getReport?) and not in the MessageManage=
r.
> >       Maybe such a move would even allows us to get rid of the
> >       MessageManager completely? I usually try to avoid singletons
> >       wherever I can...
>=20
> Yep. Having the translation step done by the DiffListener was my
> original design too. It's just a little more difficult to
> "incrementally" introduce this change, as it is necessary to update
> every DiffListener implementation. There's also the issue of what objec=
t
> holds a reference to the ResourceBundle (ie the loaded properties file).
> We certainly don't want to reload this file each time we generate a
> message. And the ApiDifference can't hold it because these are very
> short-lived objects.=20
>=20
> I think perhaps the best solution is to have each DiffListener hold the
> relevant ResourceBundle. I will have a think about this.=20
>=20
> I agree singletons should generally be avoided. However in this case
> Message objects being created as static members of classes are
> registering themselves with a "central" object, so singleton was really
> the only way to go. The fact that the MessageManager also holds the
> ResourceBundle for the translation is kind of an "after-thought".
>=20
> The MessageManager does provide a point where all the Message objects
> register themselves, so that it is possible to print out the list of al=
l
> Message objects, check there are no duplicates, check that all
> translations are present. I like having this facility available, so
> would like to keep the MessageManager and its singleton behaviour, even
> if we move the ResourceBundle elsewhere (eg on to the DiffListener
> classes).

What I've done is split out the resource-related stuff in the
MessageManager class into a new MessageTranslator class.

The MessageManager still exists, and is still a singleton. But now its
only role is as a central registry of all the (static final) Message
objects. In this sense, it is really an extension of the ClassLoader.
And as a singleton is by definition 1:1 with its ClassLoader, using a
singleton seems nice and consistent to me.

The MessageTranslator class is *not* a singleton. Each DiffReporter
class now has a MessageTranslator member which it uses as a paramter to
the ApiDifference.getReport method. This means that potentially we can
have multiple DiffReporter objects each using a different locale. I
don't know why that would ever be useful, but it's cool :-)


-------------

Because the ApiDifference constructor now requires Message objects
instead of literal report strings, there were some resulting problems
with the unit test framework.

One option would be for the unit tests to specify message IDs, eg
  ApiDifference[] expected =3D=20
  {
    new ApiDifference( new Message(1000), ....)
  }

However I think it is valuable for the tests to have a literal
message string in them, because this tests the message expansion
process; I had problems earlier with messages looking like
  "Added method {1} to class {2}"
instead of=20
  "Added method foo to class Bar".

So what I did instead was create an ExpectedDiff class, and change the
unit tests over to using:
  ExpectedDiff[] expected =3D
  {=20
    new ExpectedDiff("Added method foo to class Bar", .....)
  }

There is a method ExpectedDiff.matches(ApiDifference) which determines
whether the reported difference matches the expected one.

This also allowed removal of all the custom hashCode and equals code in
the ApiDifference class. Unfortunately, it does mean that the method
  TestDiffListener.checkExpected
isn't quite as efficient as it used to be. If you can think of a better
implementation for this, that would be nice. But in the end the new
implementation works ok, and it is only a unit test.


I hope all this is OK with you. Any and all of it can be changed...

Cheers,

Simon