[ checkstyle-Patches-1917420 ] Allow custom, parametrized check messages

View: New views

1 Messages — Rating Filter: Alert me

[ checkstyle-Patches-1917420 ] Allow custom, parametrized check messages

by SourceForge.net :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

Patches item #1917420, was opened at 2008-03-17 23:19
Message generated for change (Comment added) made by lkoe
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=397080&aid=1917420&group_id=29721

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: None
Group: None
Status: Open
Resolution: Rejected
Priority: 5
Private: No
Submitted By: Lars Koedderitzsch (lkoe)
Assigned to: Oliver Burn (oburn)
Summary: Allow custom, parametrized check messages

Initial Comment:
This patch adds a new feature to the Checkstyle core:

Custom, parametrized messages for check modules.

Rationale:
There have been many request for the eclipse-cs plugin to allow users to provide custom messages for standard Checkstyle check. A reason could be to provide textual reference to custom coding style documents or to provide more meaningful messages for some rather generic Checkstyle messages (e.g. for a naming check: "A member name must begin with a lowercase m" instead of the default "A member name must match m[A-Z][a-z]*", I think you get the idea).

The eclipse-cs plugin added such a possibility to provide static custom messages by storing the message as custom metadata in the check configuration file.
However there have been request that such custom messages should also be able to use Checkstyle message parameters - which goes beyond what we can actually achieve with the plugin.

Therefor the patch adds this support for custom, parametrizable messages to the Checkstyle core.

Please let me know what you think, especially if you see chances of this making it into Checkstyle 5.

Best regards,
Lars Kdderitzsch

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

>Comment By: Lars Koedderitzsch (lkoe)
Date: 2008-06-26 20:10

Message:
Logged In: YES
user_id=1238882
Originator: YES

Added reworked patch for custom messages as proposed on 2008-06-03 19:48.
Complete with (some) docs and tests.
Please note that with this patch the checkstyle.checkstyle ant target does
currently not succeed, because of an added parameter to the
LocalizedMessage signatures.
Can't currently decide which measures to take on this. More refactoring?
Upping the limit in the projects own Checkstyle config?

Oliver, others - please review.
Thanks,
Lars

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

Comment By: Lars Koedderitzsch (lkoe)
Date: 2008-06-26 20:06

Message:
Logged In: YES
user_id=1238882
Originator: YES

File Added: CustomMessages3.patch

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

Comment By: Lars Koedderitzsch (lkoe)
Date: 2008-06-26 16:23

Message:
Logged In: YES
user_id=1238882
Originator: YES

Working on the patch (actually its already working, just need some
tests/cleanup).
Expect results in the next days.

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

Comment By: Oliver Burn (oburn)
Date: 2008-06-17 06:52

Message:
Logged In: YES
user_id=218824
Originator: NO

OK - now I understand what you are proposing. I have no strong objection
to this change, do any of the other committers?

If not, please supply a patch and I will apply.

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

Comment By: Lars Koedderitzsch (lkoe)
Date: 2008-06-03 19:51

Message:
Logged In: YES
user_id=1238882
Originator: YES

The last comment was of course me.

Regards,
Lars

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

Comment By: Nobody/Anonymous (nobody)
Date: 2008-06-03 19:48

Message:
Logged In: NO

Of course:

<module name="LeftCurly">
<property name="option" value="nlow"/>
<message key="line.previous" value="''{0}'' must be on the previous line,
according to section 6.1 of our fance code style document"/>
<message key="line.new" value="''{0}'' must be on a new line, according
to section 6.2 of our fance code style document"/>
</module>

And now the trick:

<module name="RightCurly">
<property name="shouldStartLine" value="true"/>
<message key="line.new" value="''{0}'' should start the line, according
to section 6.3 of our fance code style document"/>
</module>

You see, it is even possible to give the same message key different custom
messages should they be shared across different checks. That - albeit
pretty constructed - example wouldn't be doable using the propertybundle
approach.

Within the eclipse-cs plugin I could offer support for defining custom
message, adding the possible message keys to the plugins Checkstyle module
metadata.

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

Comment By: Oliver Burn (oburn)
Date: 2008-06-03 08:23

Message:
Logged In: YES
user_id=218824
Originator: NO

To help me understand, could you give an example of how the LeftCurlyCheck
messages would be configured.

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

Comment By: Lars Koedderitzsch (lkoe)
Date: 2008-06-02 20:31

Message:
Logged In: YES
user_id=1238882
Originator: YES

Oliver,
thanks for the review of the patch, at least I was right with my concerns
;-)

I don't quite think that such a feature can be described as form of
localization - at least not in the sense localization is commonly used,
because these message overrides are not local specific but rather specific
to a defined module configuration in a certain Checkstyle configuration.
These custom messages possible aren't even re-usable, because a second rule
module configuration in a second Checkstyle config most likely would
require a totally different custom message.

After all the use case for those custom message is to provide
configuration specific messages over the Checkstyle default messages (aka
'Class names must start with an uppercase letter' instead of the generic
'Name XYZ must match pattern ABC').

In addition I think keeping a properties file along with their Checkstyle
configuration file would be most tiresome for users.

However, since the patch was not exactly capable of supporting
'multi-message' checks I wondered if you would support a more complex
implementation of this feature.
I was thinking along the lines of introducing a new configuration tag for
those custom message, something like

<module name="whatever">

<message value="This is a custom message {0}"/>
</module>

<module name="whatever2">

<message key="key1" value="This is a custom message {0}"/>
</module>

Implementing this would require more work than my original patch, along
with a new DTD for Checkstyle configs.
Please have a look at that proposal and tell me what you think about all
that - and if you like it I can start implementing this thing.

Best regards,
Lars

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

Comment By: Oliver Burn (oburn)
Date: 2008-05-12 08:34

Message:
Logged In: YES
user_id=218824
Originator: NO

Lars, thanks for creating this patch - enjoy your holiday.

I spent a some time studying the patch and the implementation is fine. But
I am not comfortable to apply the patch for the reason that you cited,
where confusion will happen when a Check has more than one message that it
logs. I did some rough analysis and believe around 30 Checks would fall
into the category (of the ~110 Checks that are packaged).

It is a trade-off but do not think that the confusion this change will
cause is worth the perceived benefits.

Given this, I was thinking about what a plug-in provider could do to
override the messages that are logged. I realised that there is already a
mechanism available, used to support localisation. All messages logged by
Checkstyle Checks use a resource bundle to look-up the actual message
text.

Now it is possible for a plug-in to provide a custom resource bundle to
override the message text. This would not require any change to Checkstyle.
If can even be argued that the requested feature is really just another
form of localisation of messages.

One other advantage of this approach is that it supports the fidelity of
the messages. For example, the two different messages logged by
LeftCurlyCheck could be customised differently.

Please let me know if I am way off the mark here.

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

Comment By: Lars Koedderitzsch (lkoe)
Date: 2008-05-09 23:05

Message:
Logged In: YES
user_id=1238882
Originator: YES

I re-did the patch as suggested, by adding a new property "message" to
AbstractViolationReporter.
When check log errors through the log(...) methods the message property is
transparently transported into the LocalizedMessage instance.
To not confuse the custom message with a notion of the standard message,
the new property added to LocalizedMessage is called "customMessage".
I think you'll see when looking at the changes.

I also modified those checks that already had a "message" property
(IllegalTokenTextCheck, GenericIllegalRegexpCheck, RegexpCheck) to use (or
better not use) the message property.
Instead they log their standard message like any other check and have this
message replace by the custom message transparently.
In addition a modified the logging behaviour of RegexpCheck a bit more,
because I found the current logging a bit off. Actually the "exeeded
message" (which I also externalized) did already fire when the number of
errors wasn't actually exeeded but right on the boundary. I think you will
notice this also when looking at the changed code and test case.

Still, the patch is lacking documentation, which I unfortunately cannot
manage anymore before my 2 weeks vacation. But I promise to deliver the
docs after that, unless that has been already taken care of then.

In general I still have some concerns regarding this patch:
The thing is that there are check that use more than one standard message
to report different violation conditions (e.g. LeftCurly, JavadocMethod).
The custom messages as introduced with this patch are not capable of
handling this. As there is only one custom message per check definition in
the check config, all messages reported by the affected modules are
replaced by the one custom message. This might lead to odd situations.

I am not sure if those cases should be handled at all. If they should,
however, the proposed patch is no good. In that case it would mean back to
the drawing board and think out a solution more flexible (and therefore
complex).

Please let me know what you think.
Regards,
Lars

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

Comment By: Lars Koedderitzsch (lkoe)
Date: 2008-05-09 22:44

Message:
Logged In: YES
user_id=1238882
Originator: YES

File Added: CustomMessages2.patch

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

Comment By: Oliver Burn (oburn)
Date: 2008-04-22 00:45

Message:
Logged In: YES
user_id=218824
Originator: NO

I was thinking that the new property should be called "message" instead of
"customMessage" and that any check that already supported similar
functionality change to use the property "message".

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

Comment By: Lars Koedderitzsch (lkoe)
Date: 2008-04-21 19:37

Message:
Logged In: YES
user_id=1238882
Originator: YES

Sure thing, will provide a patch with docs in the nexts days.

Regarding checks like GenericIllegalRegexp - probably you mean the
existing message property should forward to the new customMessage property
to retain compatibility with existing check configurations, right?

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

Comment By: Oliver Burn (oburn)
Date: 2008-04-20 14:13

Message:
Logged In: YES
user_id=218824
Originator: NO

Oh - and the chances of making it into version 5 are good.

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

Comment By: Oliver Burn (oburn)
Date: 2008-04-20 14:13

Message:
Logged In: YES
user_id=218824
Originator: NO

Are you able to supply the changes required to the xdocs. That would be
vital for this change.

Also, I assume that checks like GenericIllegalRegexp that support a
property "message" should be changed. Do you agree?

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

You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=397080&aid=1917420&group_id=29721

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php
_______________________________________________
Checkstyle-devel mailing list
Checkstyle-devel@...
https://lists.sourceforge.net/lists/listinfo/checkstyle-devel