Commons Lang
  1. Commons Lang
  2. LANG-362

Add ExtendedMessageFormat to org.apache.commons.lang.text

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.4
    • Component/s: None
    • Labels:
      None

      Description

      Discussed on dev@ ( http://mail-archives.apache.org/mod_mbox/commons-dev/200710.mbox/%3c590921.30381.qm@web55103.mail.re4.yahoo.com%3e ); adding here for tracking purposes and in case anyone has any serious objections to my implementation. Patch forthcoming...

      1. FormatFactory.java
        0.5 kB
        Niall Pemberton
      2. ExtendedMessageFormatTest.java
        12 kB
        Niall Pemberton
      3. ExtendedMessageFormat2.java
        12 kB
        Niall Pemberton
      4. extendedMessageFormat.patch.txt
        72 kB
        Matt Benson
      5. extendedMessageFormat.patch.txt
        68 kB
        Matt Benson
      6. DateFormatFactory.java
        3 kB
        Niall Pemberton

        Issue Links

          Activity

          Hide
          Matt Benson added a comment -

          updated from original patch to remove unnecessary constructors and make sure all ws is consumed at end of format element.

          Show
          Matt Benson added a comment - updated from original patch to remove unnecessary constructors and make sure all ws is consumed at end of format element.
          Hide
          Henri Yandell added a comment -

          General idea sounds good.

          Here's a dump of thoughts - I'll try to look more tomorrow night, and have a lot of time on Saturday.

          • Ack, lots of new classes in text. I wonder if commons.lang.format would be a tidier place.
          • MetaFormat seems to be a crux interface, but the javadoc is tiny.
          • reverse(Map) confused me - my first thought was 'HashMaps don't have order'. Invert is a better name (and you use it in the javadoc).
          • MultiFormat is a completely separate class - we should have a different JIRA item for that. I feel that this has come up before, but maybe I'm thinking of CompositeFormat. Needs a unit test focused on this, rather than the hook in in MessageFormatExtension [assuming I'm right and this is a class that can be used separately from the rest of it].
          • MultiFormat.Builder - what does 'fluent interface' mean? And what are the Builder classes for in this and NameKeyedMetaFormat, nothing uses them - until I looked in the Tests and saw them using them - not a common pattern for Lang and definitely something that if we keep it would need strong documentation.
          • DefaultMetaFormatFactory - This makes me think there is a way of replacing it with another. Is there? If not, is the Default necessary?
          • Various 'non-Javadoc' bits in there. IDE spam?

          [Sorry it's not a very deep look yet]

          Show
          Henri Yandell added a comment - General idea sounds good. Here's a dump of thoughts - I'll try to look more tomorrow night, and have a lot of time on Saturday. Ack, lots of new classes in text. I wonder if commons.lang.format would be a tidier place. MetaFormat seems to be a crux interface, but the javadoc is tiny. reverse(Map) confused me - my first thought was 'HashMaps don't have order'. Invert is a better name (and you use it in the javadoc). MultiFormat is a completely separate class - we should have a different JIRA item for that. I feel that this has come up before, but maybe I'm thinking of CompositeFormat. Needs a unit test focused on this, rather than the hook in in MessageFormatExtension [assuming I'm right and this is a class that can be used separately from the rest of it] . MultiFormat.Builder - what does 'fluent interface' mean? And what are the Builder classes for in this and NameKeyedMetaFormat, nothing uses them - until I looked in the Tests and saw them using them - not a common pattern for Lang and definitely something that if we keep it would need strong documentation. DefaultMetaFormatFactory - This makes me think there is a way of replacing it with another. Is there? If not, is the Default necessary? Various 'non-Javadoc' bits in there. IDE spam? [Sorry it's not a very deep look yet]
          Hide
          Matt Benson added a comment -
          • Ack, lots of new classes in text. I wonder if commons.lang.format would be a tidier place.

          This was the main reason I wanted to have some older [lang] hands take a look. Would CompositeFormat then be deprecated, copied to the new package, and have the old class extend the re-packaged one?

          • MetaFormat seems to be a crux interface, but the javadoc is tiny.

          I can add a little clarification there.

          • reverse(Map) confused me - my first thought was 'HashMaps don't have order'. Invert is a better name (and you use it in the javadoc).

          Noted.

          • MultiFormat is a completely separate class - we should have a different JIRA item for that. I feel that this has come up before, but maybe I'm thinking of CompositeFormat. Needs a unit test focused on this, rather than the hook in in MessageFormatExtension [assuming I'm right and this is a class that can be used separately from the rest of it].

          Yeah, CompositeFormat uses one delegate to parse, another to Format. MultiFormat iterates each operation over the first delegate that can perform it successfully. I can certainly add this as a separate issue and add its own tests.

          • MultiFormat.Builder - what does 'fluent interface' mean? And what are the Builder classes for in this and NameKeyedMetaFormat, nothing uses them - until I looked in the Tests and saw them using them - not a common pattern for Lang and definitely something that if we keep it would need strong documentation.

          http://martinfowler.com/bliki/FluentInterface.html

          I may have exaggerated a bit; I would consider the *Builders in oacl.builder to be closer to being true fluent interfaces; these Builders serve similar purposes by allowing chained method calls to quickly slap together an instance of the class they are intended to build. They're not strictly necessary, obviously.

          • DefaultMetaFormatFactory - This makes me think there is a way of replacing it with another. Is there? If not, is the Default necessary?

          The intent of the classname is to convey that this class provides static factory methods to generate, for the default or a specific Locale, a metaFormat that, when used by ExtendedMessageFormat, will produce behavior identical to java.text.MessageFormat. So it's a factory for a default metaformat. Not a default metaformat-factory, see? Any suggestions on how better to express the intent in the name? Or I could move this stuff to static methods directly on ExtendedMessageFormat.

          • Various 'non-Javadoc' bits in there. IDE spam?

          Considering most of these are probably methods overridden from classes in java.text, I probably should replace these by actual javadoc comments. For methods overridden on other classes for which source is available-and for which there are no potential IP issues in allowing inherited comments-I like allowing these to remain as they signify the javadoc has been omitted purposely.

          Thanks for your input, Henri! I will work on the simpler issues ASAP, and postpone the others until we (and anyone else who cares to jump in) discuss it a little further.

          -Matt

          Show
          Matt Benson added a comment - Ack, lots of new classes in text. I wonder if commons.lang.format would be a tidier place. This was the main reason I wanted to have some older [lang] hands take a look. Would CompositeFormat then be deprecated, copied to the new package, and have the old class extend the re-packaged one? MetaFormat seems to be a crux interface, but the javadoc is tiny. I can add a little clarification there. reverse(Map) confused me - my first thought was 'HashMaps don't have order'. Invert is a better name (and you use it in the javadoc). Noted. MultiFormat is a completely separate class - we should have a different JIRA item for that. I feel that this has come up before, but maybe I'm thinking of CompositeFormat. Needs a unit test focused on this, rather than the hook in in MessageFormatExtension [assuming I'm right and this is a class that can be used separately from the rest of it] . Yeah, CompositeFormat uses one delegate to parse, another to Format. MultiFormat iterates each operation over the first delegate that can perform it successfully. I can certainly add this as a separate issue and add its own tests. MultiFormat.Builder - what does 'fluent interface' mean? And what are the Builder classes for in this and NameKeyedMetaFormat, nothing uses them - until I looked in the Tests and saw them using them - not a common pattern for Lang and definitely something that if we keep it would need strong documentation. http://martinfowler.com/bliki/FluentInterface.html I may have exaggerated a bit; I would consider the *Builders in oacl.builder to be closer to being true fluent interfaces; these Builders serve similar purposes by allowing chained method calls to quickly slap together an instance of the class they are intended to build. They're not strictly necessary, obviously. DefaultMetaFormatFactory - This makes me think there is a way of replacing it with another. Is there? If not, is the Default necessary? The intent of the classname is to convey that this class provides static factory methods to generate, for the default or a specific Locale, a metaFormat that, when used by ExtendedMessageFormat, will produce behavior identical to java.text.MessageFormat. So it's a factory for a default metaformat. Not a default metaformat-factory, see? Any suggestions on how better to express the intent in the name? Or I could move this stuff to static methods directly on ExtendedMessageFormat. Various 'non-Javadoc' bits in there. IDE spam? Considering most of these are probably methods overridden from classes in java.text, I probably should replace these by actual javadoc comments. For methods overridden on other classes for which source is available- and for which there are no potential IP issues in allowing inherited comments -I like allowing these to remain as they signify the javadoc has been omitted purposely. Thanks for your input, Henri! I will work on the simpler issues ASAP, and postpone the others until we (and anyone else who cares to jump in) discuss it a little further. -Matt
          Hide
          Matt Benson added a comment - - edited

          Remove MetaFormat interface; make DefaultMetaFormatFactory package-private, delegate new static methods on ExtendedMessageFormat to it; add metaFormat settor to ExtendedMF to complement applyPattern() which can be called "later"; renamed AbstractMetaFormat to MetaFormatSupport; renamed AbstractDateMetaFormat to DateMetaFormatSupport (yes, a nod to Spring); made sure all inherited javadoc has something to inherit (I think).

          The packaging issue is outstanding, but since I removed an interface and made one class visible only inside its package, the impact of leaving this stuff in oacl.text is less severe; ultimately I don't really care whether they go in text or text.format however.

          EDIT: other issues addressed: s/reverse(Map)/invert(Map)/; added example documentation for MultiFormat and NamedKeyMetaFormat Builders.

          Show
          Matt Benson added a comment - - edited Remove MetaFormat interface; make DefaultMetaFormatFactory package-private, delegate new static methods on ExtendedMessageFormat to it; add metaFormat settor to ExtendedMF to complement applyPattern() which can be called "later"; renamed AbstractMetaFormat to MetaFormatSupport; renamed AbstractDateMetaFormat to DateMetaFormatSupport (yes, a nod to Spring); made sure all inherited javadoc has something to inherit (I think). The packaging issue is outstanding, but since I removed an interface and made one class visible only inside its package, the impact of leaving this stuff in oacl.text is less severe; ultimately I don't really care whether they go in text or text.format however. EDIT: other issues addressed: s/reverse(Map)/invert(Map)/; added example documentation for MultiFormat and NamedKeyMetaFormat Builders.
          Hide
          Matt Benson added a comment -

          move MultiFormat to LANG-366

          Show
          Matt Benson added a comment - move MultiFormat to LANG-366
          Hide
          Henri Yandell added a comment -

          Yeah, scratch the package thing. Moving CompositeFormat would be a pain in the arse, more than just writing a good package.html.

          Show
          Henri Yandell added a comment - Yeah, scratch the package thing. Moving CompositeFormat would be a pain in the arse, more than just writing a good package.html.
          Hide
          Matt Benson added a comment -

          Since I think I've addressed Henri's concerns and noone else voiced any, I have committed this stuff. What went in differs from the latest patch here only in that some javadoc has been further modified/cleaned up to attempt to make the intents of the new classes clearer.

          Show
          Matt Benson added a comment - Since I think I've addressed Henri's concerns and noone else voiced any, I have committed this stuff. What went in differs from the latest patch here only in that some javadoc has been further modified/cleaned up to attempt to make the intents of the new classes clearer.
          Hide
          Niall Pemberton added a comment -

          Re-opening:

          Also the class javadoc descriptions are very sparse (e.g. MetaFormatSupport only has "metaFormat support.") any chance of improving them?

          Show
          Niall Pemberton added a comment - Re-opening: Checkstyle is flagging a number of things in the new artifacts Stephen's concern raised on the dev list regarding only testing in Locale.US: http://article.gmane.org/gmane.comp.jakarta.commons.devel/99953 Also the class javadoc descriptions are very sparse (e.g. MetaFormatSupport only has "metaFormat support.") any chance of improving them?
          Hide
          Matt Benson added a comment -

          addressed issues as well as I am able.

          Show
          Matt Benson added a comment - addressed issues as well as I am able.
          Hide
          Niall Pemberton added a comment -

          I started looking at the extended message format - I like the concept but first impression is the implementation is over complex and difficult to work out what its doing. I will try to find some time to document in detail what I thinks wrong with it - but at this point I would vote -1 on a release of Lang that included it.

          Show
          Niall Pemberton added a comment - I started looking at the extended message format - I like the concept but first impression is the implementation is over complex and difficult to work out what its doing. I will try to find some time to document in detail what I thinks wrong with it - but at this point I would vote -1 on a release of Lang that included it.
          Hide
          Matt Benson added a comment -

          I'm sorry you feel that way. Please let me know what you see that could have been implemented more simply--preferably without sacrificing flexiblity, and while preserving interoperability with (i.e. extension from) java.text.MessageFormat, since the expectation is that much code in the wild is explicitly coded to MessageFormat and would thus only be usable with a subclass.

          Show
          Matt Benson added a comment - I'm sorry you feel that way. Please let me know what you see that could have been implemented more simply--preferably without sacrificing flexiblity, and while preserving interoperability with (i.e. extension from) java.text.MessageFormat, since the expectation is that much code in the wild is explicitly coded to MessageFormat and would thus only be usable with a subclass.
          Hide
          Niall Pemberton added a comment -

          OK I'll try and summarize the overall approach of your implementation:

          • ExtendedMessageFormat strips out all the "type" and "style" components from format elements leaving only the 'placeholding' argument indexes and applies that pattern to the MessageFormat it extends
          • ExtendedMessageFormat then parses the pattern again extracting the format element components and uses the "meta format" to parse each format elements type and style and create a Format for that element
          • These created formats are then set for the MessageFormat it extends.

          This is obviously a simplification, but in order to achieve the second step (i.e. creating the formats) it requires combing all the "meta format" implementations into a MultiFormat to generate all the format types currently supported by MessageFormat, plus any custom format types. So we have meta format implementations for date, time and number - plus a "factory" for these standard types. On top of that we have NameKeyedMetaFormat for anyone wanting to implement a custom format type and a couple of abstract support classes.

          • MetaFormatSupport
          • MultiFormat
          • ExtendedMessageFormat
          • NameKeyedMetaFormat
          • DefaultMetaFormatFactory
          • DateMetaFormat
          • DateMetaFormatSupport
          • NumberMetaFormat
          • TimeMetaFormat

          Working out how this all hung together took me some time and I think it will be difficult for users to grasp, difficult to support (I tried looking at the test cases to see how it worked, but its not quick to see from them either - you have to scan thru' quite a bit of code to find a real example of what you want to do).

          IMO this can be a whole lot simpler - both in terms of code quantity and complexity and in terms of concepts for users to grasp:

          I believe we only need two types to support this functionality:

          • an ExtendedMessageFormat implementation
          • a (v. simple) FormatFactory interface

          The FormatFactory interface has a single method as follows:
          Format getFormat(String name, String arguments, Locale locale);

          It works something like this:

          • create a "registry" (i.e. java.util.Map) of additional format type factories (additional to what MessageFormat already supports).
          • create an ExtendedMessageFormat instance with the "registry"
          • parse the pattern and for any format element types that have a factory registered, remove the "type" and "style" components from the pattern and pass them to the factory to create the format.
          • apply the modified pattern and set the formats for any created

          I have hacked your ExtendedMessageFormat implementation to do this and will attach it here. Its not properly tested and doesn't handle sub-formats ATM, but I believe it proves the concept and will attach in a minute

          Show
          Niall Pemberton added a comment - OK I'll try and summarize the overall approach of your implementation: ExtendedMessageFormat strips out all the "type" and "style" components from format elements leaving only the 'placeholding' argument indexes and applies that pattern to the MessageFormat it extends ExtendedMessageFormat then parses the pattern again extracting the format element components and uses the "meta format" to parse each format elements type and style and create a Format for that element These created formats are then set for the MessageFormat it extends. This is obviously a simplification, but in order to achieve the second step (i.e. creating the formats) it requires combing all the "meta format" implementations into a MultiFormat to generate all the format types currently supported by MessageFormat, plus any custom format types. So we have meta format implementations for date, time and number - plus a "factory" for these standard types. On top of that we have NameKeyedMetaFormat for anyone wanting to implement a custom format type and a couple of abstract support classes. MetaFormatSupport MultiFormat ExtendedMessageFormat NameKeyedMetaFormat DefaultMetaFormatFactory DateMetaFormat DateMetaFormatSupport NumberMetaFormat TimeMetaFormat Working out how this all hung together took me some time and I think it will be difficult for users to grasp, difficult to support (I tried looking at the test cases to see how it worked, but its not quick to see from them either - you have to scan thru' quite a bit of code to find a real example of what you want to do). IMO this can be a whole lot simpler - both in terms of code quantity and complexity and in terms of concepts for users to grasp: I believe we only need two types to support this functionality: an ExtendedMessageFormat implementation a (v. simple) FormatFactory interface The FormatFactory interface has a single method as follows: Format getFormat(String name, String arguments, Locale locale); It works something like this: create a "registry" (i.e. java.util.Map) of additional format type factories (additional to what MessageFormat already supports). create an ExtendedMessageFormat instance with the "registry" parse the pattern and for any format element types that have a factory registered, remove the "type" and "style" components from the pattern and pass them to the factory to create the format. apply the modified pattern and set the formats for any created I have hacked your ExtendedMessageFormat implementation to do this and will attach it here. Its not properly tested and doesn't handle sub-formats ATM, but I believe it proves the concept and will attach in a minute
          Hide
          Niall Pemberton added a comment -

          OK I'm attaching the following implementations

          • ExtendedMessageFormat2
          • FormatFactory
          • DateFormatFactory

          The DateFormatFactory is an example FormatFactory implementation that supports the existing "date" and "time" format types, but also adds a new "datetime" type. (btw its written as it is so that it can be used to implement Lang's FastDateFormat implementation - by just overriding the protected methods).

          So to use the new "datetime" type, all thats required is the following:

          Map registry = new HashMap();
          registry.put("datetime", new DateFormatFactory());

          String pattern = "Date:

          {0,date,short}

          Time:

          {0,time,medium}

          Date/Time:

          {0,datetime,long}

          ";
          ExtendedMessageFormat2 fmt = new ExtendedMessageFormat2(pattern, registry);

          System.out.println(fmt.format(new Object[]

          {new Date()}

          ));

          Note that the "date" and "time" types are not being handled by our custom factory - the standard MessageFormat functionality is doing that. If we wanted we could register custom formats for those standard types as well.

          Maybe I'm missing something, but is this not achieving the same goal?

          Show
          Niall Pemberton added a comment - OK I'm attaching the following implementations ExtendedMessageFormat2 FormatFactory DateFormatFactory The DateFormatFactory is an example FormatFactory implementation that supports the existing "date" and "time" format types, but also adds a new "datetime" type. (btw its written as it is so that it can be used to implement Lang's FastDateFormat implementation - by just overriding the protected methods). So to use the new "datetime" type, all thats required is the following: Map registry = new HashMap(); registry.put("datetime", new DateFormatFactory()); String pattern = "Date: {0,date,short} Time: {0,time,medium} Date/Time: {0,datetime,long} "; ExtendedMessageFormat2 fmt = new ExtendedMessageFormat2(pattern, registry); System.out.println(fmt.format(new Object[] {new Date()} )); Note that the "date" and "time" types are not being handled by our custom factory - the standard MessageFormat functionality is doing that. If we wanted we could register custom formats for those standard types as well. Maybe I'm missing something, but is this not achieving the same goal?
          Hide
          Niall Pemberton added a comment -

          P.S. This is pretty much all your code with in ExtendedMessageFormat2 with a bit of simplification and couple of additions

          Show
          Niall Pemberton added a comment - P.S. This is pretty much all your code with in ExtendedMessageFormat2 with a bit of simplification and couple of additions
          Hide
          Matt Benson added a comment -

          Niall, I've been looking at this today, and I really understand where you're coming from wrt complexity. The problem is that this is actually a much more complex task to do properly, or at least thoroughly, than it may seem at first glance. The main problem begins with the fact that preserving the original pattern to be returned by toPattern() doesn't cut it since the Formats may have been altered by a call to setFormats() (the javadoc for toPattern() actually states that the result is constructed from internal state). This in turn means AFAICT that it must be possible to render a Format back to text; hence my decision to use the metaFormat concept (parse and format). Alternatively, it might be possible to remove only the extended format information from the pattern given to the superclass, inserting as appropriate into the super toPattern() result, but keeping track of which formats (and subformats) can be handled by MessageFormat (and haven't been augmented or overridden by EMF) would be such an exercise in hard-coding that I am still not convinced that my approach isn't preferable. WDYT now?

          Show
          Matt Benson added a comment - Niall, I've been looking at this today, and I really understand where you're coming from wrt complexity. The problem is that this is actually a much more complex task to do properly, or at least thoroughly, than it may seem at first glance. The main problem begins with the fact that preserving the original pattern to be returned by toPattern() doesn't cut it since the Formats may have been altered by a call to setFormats() (the javadoc for toPattern() actually states that the result is constructed from internal state). This in turn means AFAICT that it must be possible to render a Format back to text; hence my decision to use the metaFormat concept (parse and format). Alternatively, it might be possible to remove only the extended format information from the pattern given to the superclass, inserting as appropriate into the super toPattern() result, but keeping track of which formats (and subformats) can be handled by MessageFormat (and haven't been augmented or overridden by EMF) would be such an exercise in hard-coding that I am still not convinced that my approach isn't preferable. WDYT now?
          Hide
          Niall Pemberton added a comment - - edited

          You're right it hadn't hit my radar about toPattern() reconstructing the pattern from internal state and so my suggestion reflects only half the equation, i.e. pattern-to-Format and not Format-to-pattern.

          The first thought I have about this is that one option would be to only support toPattern() for 1) existing MessageFormat compatiblity (i.e. the four "standard" built in formats) and 2) those configured through the pattern (i.e. not "non-standard" formats configured using the setFormat() methods). I believe this would be fairly straight forward to do and since EMF is providing a mechanism for using any format thru' the pattern then the use of setFormat() methods becomes unnecessary.

          Having said that even if full toPattern() support is an absolute must I still think the use of "meta formats" and the current implementation over-complicates things and my thinking goes along the following lines:

          • Using a meta "Format" implementation for the pattern<-->Format conversion is confusing in itself. I think this is where my Factory suggestion has merit since the idea of registering a "format factory" and then EMF looking-up a registered factory to create a Format is much more straight forward concept for users to grasp.
          • The current implementation does this kind of "look-up" (using NameKeyMetaFormat) but in an overcomplicated way - by creating an aggregated MultiFormat which tries to parse using delegated NameKeyMetaFormat which in turn delegates again if the format itself has arguments. Since the FormatElement part of message format's pattern is always {index, FormatType, args}

            then why have every meta-format repeatedly parse that info trying to match - my suggestion involved EMF extracting out the FormatType, looking up a registered factory for it and if found the factory then just parsing the arguments component. Surely this is simpler and less error prone than having to repeat the whole parsing of the FormatElement in every meta format?

          • If I didn't convice you with my earlier reasoning about whether full toPattern() support is really required then I think this can also be supported using the factory concept. It would reguire the "registry" to be more than a simple Map (i.e. be able to lookup a factory based on a Format, as well as name) and the FormatFactory would need additional method(s) to support re-creating the pattern - but I think that is doable as well.

          Anyway I'm not wedded to my proposal (I generally try not to just throw out criticism since IMO what counts at the ASF is not talking, but doing) and have no problem it being dismissed - but I'm not convinced that what is there currently should be included in an official release. I think the concept is great, but supporting the code and users for this I think will be a headache that we regret down the line.

          Show
          Niall Pemberton added a comment - - edited You're right it hadn't hit my radar about toPattern() reconstructing the pattern from internal state and so my suggestion reflects only half the equation, i.e. pattern-to-Format and not Format-to-pattern. The first thought I have about this is that one option would be to only support toPattern() for 1) existing MessageFormat compatiblity (i.e. the four "standard" built in formats) and 2) those configured through the pattern (i.e. not "non-standard" formats configured using the setFormat() methods). I believe this would be fairly straight forward to do and since EMF is providing a mechanism for using any format thru' the pattern then the use of setFormat() methods becomes unnecessary. Having said that even if full toPattern() support is an absolute must I still think the use of "meta formats" and the current implementation over-complicates things and my thinking goes along the following lines: Using a meta "Format" implementation for the pattern<-->Format conversion is confusing in itself. I think this is where my Factory suggestion has merit since the idea of registering a "format factory" and then EMF looking-up a registered factory to create a Format is much more straight forward concept for users to grasp. The current implementation does this kind of "look-up" (using NameKeyMetaFormat) but in an overcomplicated way - by creating an aggregated MultiFormat which tries to parse using delegated NameKeyMetaFormat which in turn delegates again if the format itself has arguments. Since the FormatElement part of message format's pattern is always {index, FormatType, args} then why have every meta-format repeatedly parse that info trying to match - my suggestion involved EMF extracting out the FormatType, looking up a registered factory for it and if found the factory then just parsing the arguments component. Surely this is simpler and less error prone than having to repeat the whole parsing of the FormatElement in every meta format? If I didn't convice you with my earlier reasoning about whether full toPattern() support is really required then I think this can also be supported using the factory concept. It would reguire the "registry" to be more than a simple Map (i.e. be able to lookup a factory based on a Format, as well as name) and the FormatFactory would need additional method(s) to support re-creating the pattern - but I think that is doable as well. Anyway I'm not wedded to my proposal (I generally try not to just throw out criticism since IMO what counts at the ASF is not talking, but doing) and have no problem it being dismissed - but I'm not convinced that what is there currently should be included in an official release. I think the concept is great, but supporting the code and users for this I think will be a headache that we regret down the line.
          Hide
          Matt Benson added a comment - - edited

          If we proceed from the supposition that full dynamic toPattern() support is required, it seems to me that creating a different interface to convert Formats to/from strings is redundant, and it was this line of thinking that led me to what I saw as an epiphany: that that's what a Format is.

          HOWEVER, I must admit that your reasoning about why toPattern() is far less important - once we have established that EMF obviates the need for altering the formats at runtime - was fairly convincing. The main object now being trying to achieve a compromise, would you agree that if setFormats(), setFormat(), and setFormatByArgumentIndex() can all be overridden to throw UnsupportedOperationException? Explicitly blocking these would, IMO, make returning the original pattern unchanged from toPattern() perfectly fine. Then we could go with the EMF2 approach. Are we getting closer to consensus?

          Show
          Matt Benson added a comment - - edited If we proceed from the supposition that full dynamic toPattern() support is required, it seems to me that creating a different interface to convert Formats to/from strings is redundant, and it was this line of thinking that led me to what I saw as an epiphany: that that's what a Format is. HOWEVER, I must admit that your reasoning about why toPattern() is far less important - once we have established that EMF obviates the need for altering the formats at runtime - was fairly convincing. The main object now being trying to achieve a compromise, would you agree that if setFormats(), setFormat(), and setFormatByArgumentIndex() can all be overridden to throw UnsupportedOperationException? Explicitly blocking these would, IMO, make returning the original pattern unchanged from toPattern() perfectly fine. Then we could go with the EMF2 approach. Are we getting closer to consensus?
          Hide
          Niall Pemberton added a comment -

          Yes that would greatly simplify things - how does that sit though with something thats completely compatible with MessageFormat? Works for me, but I thought you wanted a drop in replacement?

          Show
          Niall Pemberton added a comment - Yes that would greatly simplify things - how does that sit though with something thats completely compatible with MessageFormat? Works for me, but I thought you wanted a drop in replacement?
          Hide
          Matt Benson added a comment -

          IMO that's not important. I think those methods are intended for setup rather than a runtime (i.e. after the dropping-in has taken place). I would be quite surprised to find client code that used these methods at a level lower than configuration, where I would expect the substitution to take place. If it becomes an issue we could add an option to ignore the calls rather than throwing Exceptions.

          Show
          Matt Benson added a comment - IMO that's not important. I think those methods are intended for setup rather than a runtime (i.e. after the dropping-in has taken place). I would be quite surprised to find client code that used these methods at a level lower than configuration, where I would expect the substitution to take place. If it becomes an issue we could add an option to ignore the calls rather than throwing Exceptions.
          Hide
          Matt Benson added a comment -

          I have, I believe, resolved the issue such that the compromises outlined by myself and Niall have been reached. The pattern rendition is fixed at construction: the super implementation of toPattern() is used for the patterns resolved by the superclass, and toPattern for custom patterns is handled in the extension, removing superfluous whitespace as does the super implementation (this was the part I had to find the time to finish).

          Show
          Matt Benson added a comment - I have, I believe, resolved the issue such that the compromises outlined by myself and Niall have been reached. The pattern rendition is fixed at construction: the super implementation of toPattern() is used for the patterns resolved by the superclass, and toPattern for custom patterns is handled in the extension, removing superfluous whitespace as does the super implementation (this was the part I had to find the time to finish).
          Hide
          Niall Pemberton added a comment -

          OK I have a couple of points

          • Now the setFormats(), setFormat(), and setFormatByArgumentIndex() all throw UnsupportedOperationException I don't see the need for the logic to re-create the pattern - much simpler to just cache the pattern the EMF was created with and return that value in toPattern() - or am I missing something?
          • the test cases could (and IMO should) be simpler - currently there are four classes to test EMF (AbstractMessageFormatTest, ExtendedMessageFormatBaselineTest, MessageFormatExtensionTest. and MessageFormatTest) and I don't see why we don't just have one ExtendedMessageFormatTest). Tests are often a good way to look at how something works - so the simpler the better both for those maintaining it and users wanting to understand how to use it.
          Show
          Niall Pemberton added a comment - OK I have a couple of points Custom sub-formats are not supported - so if I embed a "custom" format in a choice format it throws an IllegalArgumentException. Do we want to support that scenario or document clearly that it doesn't work? (I did make this point in https://issues.apache.org/jira/browse/LANG-362?focusedCommentId=12564290#action_12564290 ) Now the setFormats(), setFormat(), and setFormatByArgumentIndex() all throw UnsupportedOperationException I don't see the need for the logic to re-create the pattern - much simpler to just cache the pattern the EMF was created with and return that value in toPattern() - or am I missing something? the test cases could (and IMO should) be simpler - currently there are four classes to test EMF (AbstractMessageFormatTest, ExtendedMessageFormatBaselineTest, MessageFormatExtensionTest. and MessageFormatTest) and I don't see why we don't just have one ExtendedMessageFormatTest). Tests are often a good way to look at how something works - so the simpler the better both for those maintaining it and users wanting to understand how to use it.
          Hide
          Niall Pemberton added a comment -

          Attaching a test case for ExtendedMessageFormat that I think should replace the existing four test classes.

          Note this includes a couple of tests for custom sub-formats which currently fail.

          Show
          Niall Pemberton added a comment - Attaching a test case for ExtendedMessageFormat that I think should replace the existing four test classes. Note this includes a couple of tests for custom sub-formats which currently fail.
          Hide
          Matt Benson added a comment -

          OK I have a couple of points
          Custom sub-formats are not supported - so if I embed a "custom" format in a choice format it throws an IllegalArgumentException. Do we want to support that scenario or document clearly that it doesn't work?
          (I did make this point in https://issues.apache.org/jira/browse/LANG-362?focusedCommentId=12564290#action_12564290)

          I had missed your having made that point. I will look into this and see how heinous it is.

          Now the setFormats(), setFormat(), and setFormatByArgumentIndex() all throw UnsupportedOperationException I don't see the need for the logic to re-create the pattern - much simpler to just cache the pattern the EMF was created with and return that value in toPattern() - or am I missing something?

          Yes--jt.MF strips unnecessary whitespace and further may mangle e.g. date patterns if it can't differentiate between subformats (usu. because you've specified a pattern or style that collides with the system default); recreating the pattern allows the subclass to have output as close as possible to what would be created by the superclass (identical when no custom subformats are used).

          the test cases could (and IMO should) be simpler - currently there are four classes to test EMF (AbstractMessageFormatTest, ExtendedMessageFormatBaselineTest, MessageFormatExtensionTest. and MessageFormatTest) and I don't see why we don't just have one ExtendedMessageFormatTest). Tests are often a good way to look at how something works - so the simpler the better both for those maintaining it and users wanting to understand how to use it.

          I will concede that now that we are not reinventing the proverbial wheel the existing test cases are overkill.

          Show
          Matt Benson added a comment - OK I have a couple of points Custom sub-formats are not supported - so if I embed a "custom" format in a choice format it throws an IllegalArgumentException. Do we want to support that scenario or document clearly that it doesn't work? (I did make this point in https://issues.apache.org/jira/browse/LANG-362?focusedCommentId=12564290#action_12564290 ) I had missed your having made that point. I will look into this and see how heinous it is. Now the setFormats(), setFormat(), and setFormatByArgumentIndex() all throw UnsupportedOperationException I don't see the need for the logic to re-create the pattern - much simpler to just cache the pattern the EMF was created with and return that value in toPattern() - or am I missing something? Yes--jt.MF strips unnecessary whitespace and further may mangle e.g. date patterns if it can't differentiate between subformats (usu. because you've specified a pattern or style that collides with the system default); recreating the pattern allows the subclass to have output as close as possible to what would be created by the superclass (identical when no custom subformats are used). the test cases could (and IMO should) be simpler - currently there are four classes to test EMF (AbstractMessageFormatTest, ExtendedMessageFormatBaselineTest, MessageFormatExtensionTest. and MessageFormatTest) and I don't see why we don't just have one ExtendedMessageFormatTest). Tests are often a good way to look at how something works - so the simpler the better both for those maintaining it and users wanting to understand how to use it. I will concede that now that we are not reinventing the proverbial wheel the existing test cases are overkill.
          Hide
          Niall Pemberton added a comment -

          OK I've commited my test case:

          http://svn.apache.org/viewvc?view=rev&revision=631639

          Mind if I delete the other four?

          • AbstractMessageFormatTest
          • ExtendedMessageFormatBaselineTest
          • MessageFormatExtensionTest
          • MessageFormatTest
          Show
          Niall Pemberton added a comment - OK I've commited my test case: http://svn.apache.org/viewvc?view=rev&revision=631639 Mind if I delete the other four? AbstractMessageFormatTest ExtendedMessageFormatBaselineTest MessageFormatExtensionTest MessageFormatTest
          Hide
          Matt Benson added a comment -

          Go for it!

          Show
          Matt Benson added a comment - Go for it!
          Hide
          Henri Yandell added a comment -

          What's left to get this resolved?

          Show
          Henri Yandell added a comment - What's left to get this resolved?
          Hide
          Niall Pemberton added a comment -

          I'm happy with the public API were now exposing to the users and while I haven't looked in detail at the code, it is greatly simplified (thanks Matt!) and writing a test case for it gives me confidence that it works pretty well. So the only outstanding issue is wether we want to suport custom Format implementations in sub-formats (i.e. choice) or just document that they are not supported. I'm happy either way so I would say if Matt has the time and inclination to do it soon then great - otherwise lets just document they are not currently supported (in the ExtendedMessageFormat javadocs) and leave if for a later release.

          Show
          Niall Pemberton added a comment - I'm happy with the public API were now exposing to the users and while I haven't looked in detail at the code, it is greatly simplified (thanks Matt!) and writing a test case for it gives me confidence that it works pretty well. So the only outstanding issue is wether we want to suport custom Format implementations in sub-formats (i.e. choice) or just document that they are not supported. I'm happy either way so I would say if Matt has the time and inclination to do it soon then great - otherwise lets just document they are not currently supported (in the ExtendedMessageFormat javadocs) and leave if for a later release.
          Hide
          Matt Benson added a comment -

          Okay, it's on my TODO list for today. I'll look into the difficulty level of supporting sub-subformats in choice and possibly custom subformats, but if it's simply too hairy I'll add a new post-2.4 issue for that.

          Show
          Matt Benson added a comment - Okay, it's on my TODO list for today. I'll look into the difficulty level of supporting sub-subformats in choice and possibly custom subformats, but if it's simply too hairy I'll add a new post-2.4 issue for that.
          Hide
          Matt Benson added a comment -

          I didn't get to it until today. And I was close... but, in the end, it appears that the API of MessageFormat makes it simply impossible to support additional embedded subformats in a ChoiceFormat. This sucks.

          Anyway, I will document this and remove the testcases. If anyone else cares to look into this in the future, be my guest. But as far as I can tell this requires (possibly very simple) changes in java.text.MessageFormat. I may add something to the bug parade, though my past experiences with this (admittedly pre-GPL Java) haven't been great.

          Show
          Matt Benson added a comment - I didn't get to it until today. And I was close... but, in the end, it appears that the API of MessageFormat makes it simply impossible to support additional embedded subformats in a ChoiceFormat. This sucks. Anyway, I will document this and remove the testcases. If anyone else cares to look into this in the future, be my guest. But as far as I can tell this requires (possibly very simple) changes in java.text.MessageFormat. I may add something to the bug parade, though my past experiences with this (admittedly pre-GPL Java) haven't been great.
          Hide
          Niall Pemberton added a comment -

          Please don't remove the test cases - they may save some effort in the future if this gets implemented

          Show
          Niall Pemberton added a comment - Please don't remove the test cases - they may save some effort in the future if this gets implemented
          Hide
          Matt Benson added a comment -

          It seems somewhat misleading to keep running do-nothing tests. I figured having them around in rcs was good enough, though I'll concede someone coming in later might not notice them right away. What if I simply comment or similarly disable them?

          Also, if you could verify when you run the failing tests that the stacktrace contains a call to MessageFormat's constructor, I could submit my RFE to the Sun... I'm running on OSX now. If not, I'll do it later.

          For information's sake, I plan to suggest replacing new MessageFormat(pattern) with MessageFormat mf = (MessageFormat) this.clone(); mf.applyPattern(pattern); in the ChoiceFormat handling would suffice, though I don't plan to go through all the submit-a-fix-to-Sun rigmarole...

          Show
          Matt Benson added a comment - It seems somewhat misleading to keep running do-nothing tests. I figured having them around in rcs was good enough, though I'll concede someone coming in later might not notice them right away. What if I simply comment or similarly disable them? Also, if you could verify when you run the failing tests that the stacktrace contains a call to MessageFormat's constructor, I could submit my RFE to the Sun... I'm running on OSX now. If not, I'll do it later. For information's sake, I plan to suggest replacing new MessageFormat(pattern) with MessageFormat mf = (MessageFormat) this.clone(); mf.applyPattern(pattern); in the ChoiceFormat handling would suffice, though I don't plan to go through all the submit-a-fix-to-Sun rigmarole...
          Hide
          Niall Pemberton added a comment - - edited

          OK commenting out works for me - I'll do that unless you beat me to it.

          The failing stack trace for the test is below (snipped after ExtendedMessageFormatTest) - java version 1.5.0_07

          java.lang.IllegalArgumentException: unknown format type at
          at java.text.MessageFormat.makeFormat(MessageFormat.java:1426)
          at java.text.MessageFormat.applyPattern(MessageFormat.java:447)
          at java.text.MessageFormat.<init>(MessageFormat.java:365)
          at java.text.MessageFormat.subformat(MessageFormat.java:1214)
          at java.text.MessageFormat.format(MessageFormat.java:825)
          at java.text.Format.format(Format.java:133)
          at org.apache.commons.lang.text.ExtendedMessageFormatTest.testExtendedWithChoiceFormat(ExtendedMessageFormatTest.java:138)

          Show
          Niall Pemberton added a comment - - edited OK commenting out works for me - I'll do that unless you beat me to it. The failing stack trace for the test is below (snipped after ExtendedMessageFormatTest) - java version 1.5.0_07 java.lang.IllegalArgumentException: unknown format type at at java.text.MessageFormat.makeFormat(MessageFormat.java:1426) at java.text.MessageFormat.applyPattern(MessageFormat.java:447) at java.text.MessageFormat.<init>(MessageFormat.java:365) at java.text.MessageFormat.subformat(MessageFormat.java:1214) at java.text.MessageFormat.format(MessageFormat.java:825) at java.text.Format.format(Format.java:133) at org.apache.commons.lang.text.ExtendedMessageFormatTest.testExtendedWithChoiceFormat(ExtendedMessageFormatTest.java:138)
          Hide
          Matt Benson added a comment -

          Thanks for the stacktrace... I'll have to finish submitting the RFE later since they need BRIEF code.

          Are we all done here then?

          Show
          Matt Benson added a comment - Thanks for the stacktrace... I'll have to finish submitting the RFE later since they need BRIEF code. Are we all done here then?
          Hide
          Niall Pemberton added a comment -

          I think so

          Show
          Niall Pemberton added a comment - I think so

            People

            • Assignee:
              Matt Benson
              Reporter:
              Matt Benson
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development