StringToPeriodConverter is at odds with other converters at the moment (being public and not flagged final).
2.3.0 is out so that would be a breaking change.
Comment From: snicoll
Discussing this with @jhoeller a bit more, I think we should expand the scope a bit. I thought initially that it is possible to register more than one Converter for the same pair and return null if the input can't be handled to let another converter try. That would be an elegant solution if one had to support a slightly different format, registering theirs before ours. Turns out only one converter per pair is supported.
If our converter is not a straightforward call, we should consider:
- Making the converter
public - Moving the conversion logic to a public API so that others can reuse it in their custom implementation.
Comment From: OrangeDog
The conversion logic in this case is almost public: PeriodStyle.detectAndParse(String, ChronoUnit).
However, that doesn't account for the @PeriodFormat or @PeriodUnit annotations that the converter also uses.
Comment From: philwebb
I think for now we should risk making this one package private in 2.3.1. The chances of anyone extending it yet are pretty slim.
Comment From: philwebb
If our converter is not a straightforward call, we should consider...
My feeling is that the converters are generally not designed to be subclasses or extended so we should keep them package private and not offer any public API. At least, not until someone specifically asks us to.
Comment From: snicoll
At least, not until someone specifically asks us to.
@OrangeDog did for the duration support on Gitter which triggered this issue when I was telling him the exact same thing and noticed the Period one was inconsistent.
Comment From: philwebb
I missed that conversation, is there more context for why the override was required?
Comment From: OrangeDog
I was doing this
one had to support a slightly different format, registering theirs before ours.
and wanted a public constructor so I could delegate to an instance independently of a ConversionService.
And then I found DurationStyle.detectAndParse(String, ChronoUnit) and used that.
Comment From: snicoll
Alright so case closed and let's make this one consistent. Thanks both.
Comment From: OrangeDog
Why was the @since 2.3.0 removed? I thought it was accurate.
Comment From: snicoll
We don't have @since tag for package protected class.