o.a.l.messages should be moved to core [LUCENE-1887]

[asfimport]

contrib/queryParser contains an org.apache.lucene.messages package containing some generallized code that (claims in it's javadocs) is not specific to the queryParser.

If this is truely general purpose code, it should probably be moved out of hte queryParser contrib – either into it's own contrib, or into the core (it's very small)

EDIT: alternate suggestion to rename package to fall under the o.a.l.queryParser namespace retracted due to comments in favor of (eventually) promoting to it's own contrib

---

Migrated from LUCENE-1887 by Chris M. Hostetter (@hossman), resolved Sep 10 2009 Attachments: lucene-1877-new.patch, lucene-1877-new-v2.patch, LUCENE-1887.patch, LUCENE-1887-javadocs-all.patch

[asfimport]

Adriano Crestani (migrated from JIRA)

These classes have not relation with the queryparser code, the queryparser only uses it. Maybe in future other parts of lucene may start using it. So I vote to leave it outside queryParser package for now, and when other parts of lucene start using it, we can think about moving it to core.

[asfimport]

Luis Alves (migrated from JIRA)

+1 to keep outside parser.

These small utility classes are designed to handle Message translation, for the queryparser but can be used by any other component in lucene that wants to display translated resources.

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

I agree it would be nice to have a package for localized messages in general. In the future this leaves open the door for localization of other parts of lucene.

since it uses MessageFormat etc, I think it would be very reusable.

[asfimport]

Mark Miller (@markrmiller) (migrated from JIRA)

The window is closing on this one - the current code is 1.5 - do we want to convert to 1.4 and move to core? Move to messages contrib? Else it will remain the same.

[asfimport]

Chris M. Hostetter (@hossman) (migrated from JIRA)

These classes have not relation with the queryparser code, the queryparser only uses it.

that seems like a pretty strong argument to promote it into it's own contrib ... no other contrib is going to start depending on queryParser just to get access to a messages class – And if we wait until 3.x to move it to it's own contrib, we make a lot of headaches for any users who start(ed) using the queryParser contrib in 2.9, because all of hte sudden their code will stop working at runtime because the classes can't be found.

(it's an easy problem to fix: tell them to use the new jar as well, but it reflects badly on the project when people encounter annoyances like that when upgrading)

That said: i'm not going to argue that hard if no more closely involved in the contrib thinks it's worth moving .. removing 2.9 fix-for.

[asfimport]

Luis Alves (migrated from JIRA)

The original idea is to have messages and the new queryparser moved into core on 3.0. But this was discussed in the mailing list and the community thinks it's better to give it some more time for the API's on the new QP to get stable and allow more time for review of the new queryparser.

I think there is an understanding to move the new QueryParser to core, when 3.1 is released.

When the new messages and the queryparser is moved to core, it will be more usable. If we keep it on contrib, in my opinion we should leave it as is for release 2.9 and 3.0

We should re-look at this issue when 3.0 is done, by moving message and the queryparser to core.

[asfimport]

Luis Alves (migrated from JIRA)

The window is closing on this one - the current code is 1.5 - do we want to convert to 1.4 and move to core? Move to messages contrib? Else it will remain the same.

+1 I am OK with making the o.a.l.messages.* 1.4 compatible and move it to core in 2.9.

NOTE: We also would have to move the testcases for the o.a.l.messsages.*

[asfimport]

Adriano Crestani (migrated from JIRA)

+1 to make it 1.4 compatible and move it to core in 2.9

+1 to move it to another contrib project called, e.g. "messages"

I think Hoss made his point about not leaving this code in queryparser contrib project and I agree.

[asfimport]

Mark Miller (@markrmiller) (migrated from JIRA)

Can you do it right now? So I can do rc4 tonight?

[asfimport]

Adriano Crestani (migrated from JIRA)

Hi Mark,

Can you do it right now? So I can do rc4 tonight?

Who is "you"? And do what, move to core or move to a new contrib project?

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

first do:

svn move contrib/queryparser/src/java/org/apache/lucene/messages src/java/org/apache/lucene
svn move contrib/queryparser/src/test/org/apache/lucene/messages src/test/org/apache/lucene

then apply patch

[asfimport]

Luis Alves (migrated from JIRA)

I created a patch it should apply clean in main.

I'm looking at Robert's patch, only notice he posted after I created the patch :)

[asfimport]

Mark Miller (@markrmiller) (migrated from JIRA)

Okay - just let me know which to go with

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

Luis the only real difference from yours is i used Object[] for the varags like the 1.4 MessageFormat

do we really need to deprecate the Object[] when it becomes ... in 1.5? sun didnt. I am java5 brain-dead :) this is documented to be compatible: http://java.sun.com/j2se/1.5.0/docs/guide/language/varargs.html

[asfimport]

Luis Alves (migrated from JIRA)

I looked at Robert's patch it is very similar to mine, but Robert changed the testcases and the queryparser code to use a new method signature.

My patch added more utility methods (deprecated) on NLS and MessageImpl and does not require any changes the QP and the tescases.

I prefer my approach because it will be compatible to jdk1.5, once we remove the utility methods from these classes and revert back to the jdk 1.5 syntax in 3.0.

Robert please take a look at my patch and see if you like it.

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

Luis, yeah, the difference is your patch has the deprecated methods.

in my patch there are no deprecated methods, it uses Object[] instead of varargs. but these are compatible to jdk1.5: http://java.sun.com/j2se/1.5.0/docs/guide/language/varargs.html so you can change it to ... (varargs) and it will be backwards compatible, without any deprecation. then at your leisure, once on jdk1.5, you can remove the new Object[] {}, it will be autoboxed.

[asfimport]

Luis Alves (migrated from JIRA)

I agree with you on the compatibility, I'll remove the deprecated from the getLocalizedMessage method on NLS class and submit again

public static String getLocalizedMessage(String key, Locale locale, Object[] args)

I just think it is ugly having to create a Object array for every call to the Message classes. See Example below.

Original or with my patch

throw new ParseException(new MessageImpl(QueryParserMessages.INVALID_SYNTAX_ESCAPE_NONE_HEX_UNICODE, c));

Example using your patch:

throw new ParseException(new MessageImpl(QueryParserMessages.INVALID_SYNTAX_ESCAPE_NONE_HEX_UNICODE, new Object[]{c}));

This is not a big thing for me. If we don't care that the users of these classes in 2.9 are required to create the object arrays for most method calls, then I am totally ok with your patch.

We can clean the new QueryParser code, of those object arrays in 3.0.

Robert or Mark pick any of the patches, they both look good to me. Need to go home, I'll check the issue later.

[asfimport]

Luis Alves (migrated from JIRA)

I updated the my patch to remove the deprecated from

public static String getLocalizedMessage(String key, Locale locale, Object[] args)

I'm also ok with Robert patch, they are almost identical.

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

I just think it is ugly having to create a Object array for every call to the Message classes.

my reasoning here was that this is how MessageFormat works in java 1.4, since this is a wrapper around MessageFormat, it is consistent with MessageFormat.

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

Luis, if we go with your patch, I think we should add MessageImpl(Object[]) constructor back , not deprecated, to be changed to MessageImpl(Object ...) in 1.5 otherwise it is limited to 3 arguments :)

[asfimport]

Mark Miller (@markrmiller) (migrated from JIRA)

I agree on the consistency front - Robert, I'll go with yours it looks - is it ready, or need to make any last changes?

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

I agree on the consistency front - Robert, I'll go with yours it looks - is it ready, or need to make any last changes?

i think it is ready, all the tests pass.

[asfimport]

Mark Miller (@markrmiller) (migrated from JIRA)

Thanks a million guys.

[asfimport]

Uwe Schindler (@uschindler) (migrated from JIRA)

One small thing: The javadocs-all still list the messages package in queryparser contrib. I will attach a patch.

[asfimport]

Uwe Schindler (@uschindler) (migrated from JIRA)

Revision: 813268

[asfimport]

Luis Alves (migrated from JIRA)

I was not able to read the thread late yesterday :(.

But great work, it's good to see the message classes in main. Even if is not my patch I like it :)

Thanks guys