As SimpleUnitFormat can be customized, allow consumers to have their own instances.

[andi-huber]

Relates to the use-case as adressed with https://github.com/geotools/geotools/pull/3232.

That is, library providers potentially want to implement customized version of a UnitFormat by not starting from scratch, but reusing what the SimpleUnitFormat comes pre-configured with.

The suggested solution is to simply allow consumers of Indriya to have their own instances of SimpleUnitFormat, which they may customize to their own needs. This has no side-effect on Indriya, eg. it does not affect the singletons provided by Indriya.

[keilw]

I think an alternate getInstance() variation under a different name feels good, looking at many java.text.Format classes like NumberFormat with a default getInstance() beside several others like getCurrencyInstance() etc.

@andi-huber Please if you can try to resolve the merge conflicts between the current master branch and that PR branch. Make sure to clearly explain in the JavaDoc what each of them does to avoid misinformation or confusion about each of them. BTW the current getInstance() JavaDoc is also misleading, it was derived from one of those java.text.Format classes once (and for a long time AbstractFormat and most other formats actually derived from that) especially there is no locale involved, so @return the default unit format (locale sensitive). is wrong.

Ultimately DefaultFormat and ASCIIFormat should ideally be made package-local, the latter is already final so it can't be extended anyway. And while calling various get*Instance() flavors is

If you take com.fasterxml.jackson.databind.util.StdDateFormat from Jackson Data Binding as an example it provides two similar static factory methods getBlueprintRFC1123Format() ("Method for getting the globally shared DateFormat instance") and getISO8601Format(TimeZone) ("Method for getting a non-shared DateFormat instance that uses specified timezone and can handle simple ISO-8601 compliant date format") So pointing out, that getInstance() (both with and without argument) provides a shared instance and getNewInstance() provides a non-shared new instance for specific purposes that need a clean slate is important in the JavaDoc.

[andi-huber]

Thanks @keilw - I'll see that I can improve the java-doc a bit. Also I don't feel the urge to rush this. If there are issues left to address or polish, I'm happy to consider that. We can think about slight variations of the naming so eg. instead of getNewInstance we could be more specific and call these ...

newDefaultFormat or createDefaultFormatwith variant newAsciiFormat or createAsciiFormat. The naming I personally don't care too much, so I'm happy if you pick your preference.

[keilw]

@andi-huber Naming them getXYZInstance() along the lines of the JDK Format classes really seems best and if it creates a new instance as opposed to the existing "shared" instance factory getNewInstance() feels OK. The control whether it applies to the default (Unicode) or ASCII variant happens via the enum type, so it would be confusing to have 2 or more different methods here and just one passing an enum for the other case.

I'm still not sure how these factory methods fit extending from SimpleUnitFormat (because there whether they offer a constructor, builder or factory class or similar static method as its own, SimpleUnitFormat.get*Instance() won't help them, it never creates a subclass defined elsewhere and all the static initializers are called anyway, so all you get from getNewInstance() is losing every label a different system like SI, US Customary etc. ) but it seems you found a way to convince them what I also suggested more than once, that they may not need a separate UnitFormat but probably more than one instance or alias of a Unit. If this extra pair of methods allows them to control the labels for those, it would be "Mission Accomplished".

[andi-huber]

@keilw - So we stick with the original plan of how we name those 2 new methods, good.

I was wondering, as you mentioned that I would need to clean up merge conflicts. I don't (and did not) see any conflicts with the master branch at GitHub. I branched off from master yesterday to create this PR. Could it be, maybe you have some local changes not yet pushed to the GitHub repo. Please double check and let me know.

[keilw]

Sounds good, if you can merge it without conflicts that's great. Here in the web interface it said it would not work without resolving some conflicts.

[andi-huber]

If we did not introduce any regression, its safe to assume this issue is fixed.

[keilw]

It would be great to have a few JUnit tests demonstrating the effect of calling either of those, but in some cases it may even take a demo app or extension module like SI Units etc. to really show.

[andi-huber]

Sure, though I'm pretty confident that these 2 new methods are safe. My comment/concern was rather with the existing methods, but we have these covered, so all good I'd say.