Re: [Clirr-devel] Message framework

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

On Wed, 2004-06-30 at 08:47, Lars K=FChne wrote:
> Simon Kitching wrote:
>=20
> >Hi Lars,
> >
> >Ok, message framework all committed. Please have a look and let me kno=
w
> >what you think.
> >
> > =20
> >
>=20
> In general the approach of using message IDs is great, and I can see=20
> where you are heading with the docs.
>=20
> A few things concerning the concrete implementation:
>=20
>     * I would probably have kept all Message instances in the Message
>       class itself (typesafe enum idiom), not in the MethodSetCheck. Th=
e
>       event package will be part of clirr's public API (for IDE plugins=
)
>       and I think we don't want to have a public constructor for
>       Messages. Typesafe enum also provides a central point of referenc=
e
>       for all possible changes Clirr can find.

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 to
any check, the Message class needs to gets updated. With the design I
committed, that doesn't need to happen.

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.

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


>     * For testing I think it would be better if the translation output
>       language would be controlled by the DiffListener (add a Locale
>       param do ApiDifference.getReport?) and not in the MessageManager.
>       Maybe such a move would even allows us to get rid of the
>       MessageManager completely? I usually try to avoid singletons
>       wherever I can...

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

I think perhaps the best solution is to have each DiffListener hold the
relevant ResourceBundle. I will have a think about this.=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".

The MessageManager does provide a point where all the Message objects
register themselves, so that it is possible to print out the list of all
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).

>     * Using english language properties files without language extensio=
n
>       (i.e. x.properties instead of x_en.properties) does bring some
>       nasty surprises, see
>       https://sourceforge.net/tracker/index.php?func=3Ddetail&aid=3D594=
469&group_id=3D29721&atid=3D397078
>       <>I have a solution that will work without copying the properties
>       file during the build (like we do in checkstyle). You just need t=
o
>       use a valid class name as the ressource name, e.g.
>       EventMessages_en.properties, I can do the rest.
>       <><>=20

I understand the problem described by your email (and yes that is
nasty!). I don't understand your comment above, though.

Isn't the correct solution just to copy event_messages.properties to
event_messages_en.properties as part of the build?

With that:
 * if the user asks for "en" then "en" is found.
 * if the user asks for an unsupported locale then they get the
   properties for the machine's default locale (if it exists)
 * if there are no properties for the machine's default locale,
   then they get the base version (which is also english).

So what do you mean by "You just need to use a valid class name as the
resource name"? Maybe it is just easier if you commit the fix then I can
see what you meant :-)


>=20
> Hope this doesn't come across too negative... as I said, I think this i=
s=20
> great work!

No problems at all. It's great to get feedback. One of the things I miss
with my current job is the chance to debate design a little; I
essentially work alone on the projects I am responsible for.

Your comments are all high-quality too; I'm happy to get them. And if I
disagree, I'll simply let you know :-)

>=20
> >I've only updated the MethodSetCheck class to use it, so that if there
> >are changes I don't need to redo/revert everything. If you like it, th=
en
> >I will go ahead and convert the other check classes over.
> >
> > =20
> >
>=20
> Go ahead, we can clean up the above points later.
>=20

Will do!

Cheers,

Simon