Advice in migrating GeoTools away from meddling in UnitFormat's innards

[soc]

I'm looking into this GeoTools issue which is caused by GeoTools trying to extract private data from this library.

While this has worked with units-1.0, after GeoTools migrated to units-2.0 this now fails when running under the module system.

The core part is in GeoToolsUnitFormat in which GeoTools tries to access non-public fields of UnitFormat.

Ideally, GeoTools would stop doing reflection against this library, but it seems that the public API does not really offer an alternative.

I'm hoping you could offer some advice on how GeoTools can adapt to use this library more "correctly".

Any hints and help would be appreciated!

[soc]

As suggested in the linked file, there are two main routes, either:

1. Provide some way to get an UnitFormat instance with the same setup as SimpleUnitFormat#getInstance() that allows for extension and can, unlike the libraries we depend on, actually parse back what it formatted (see https://github.com/unitsofmeasurement/uom-se/issues/201), or

2. make unitFor (and probably nameFor) public, such that we can keep scraping data out of UnitFormat instances manually.

[keilw]

Did you check with @desruisseaux? E.g. his alternate implementation Seshat extends java.text.Format instead. Wouldn't it be easiser to extend AbstractUnitFormat instead of SimpleUnitFormat? While that is also an abstract base class for the Default/Unicode or ASCII specializations, it has many static methods that have a global effect. Not sure, if that was desired? A few methods like protected String nameFor(Unit<?> unit) in SUF are protected already, so which one are you missing? The getInstance() method of SUF does not really do anything except picking the default instance (unless you select ASCII) most of the initialization is done via static initializers and calling label() in a subclass would therefore also have a side-effect on SimpleUnitFormat.

Not sure, if this is intentional, as long as the users and using classes of GeoTools take that into consideration, it may be beneficial, but it has to be handled with care because the label() method does everyhing you ask for in 2. directly to SimpleUnitFormat.

So while SUF is abstract for a reason right now and allows extending (the real purpose was e.g. a different character set beside ASCII and Unicode) what you have in mind may better work on top of AbstractUnitFormat. You see e.g. the UCUMFormat using mechanisms not so different from LocalUnitFormat or EBNFUnitFormat in the RI but in its own separate subsystem without any side-effects.

WDYT?

[jodygarnett]

Indeed we are familiar with @desruisseaux who worked previously on the geotools project.

For this topic the library is looking for guidance on how best to integrate with indriya, it is very possible that there is a more appropriate base class to start out from that what we are presently using.

aside: This is our third time migrating to a new implementation (keeping up as the units api and reference implementations evolve over time). Indeed each time a volunteer with different level of experience has done the work so we miss some continuity on this activity.

[andi-huber]

We've now changed SimpleUnitFormat ('master' branch), so you can simply write ...

public abstract class GeoToolsUnitFormat extends SimpleUnitFormat {

    private static BaseGT2Format INSTANCE;

    static {
        INSTANCE = new BaseGT2Format();
    }

    public static BaseGT2Format getInstance() {
        return INSTANCE;
    }

    /**
     * Base class that just copies {@link UnitFormat} default instance contents
     */
    protected static class BaseGT2Format extends DefaultFormat {

        public BaseGT2Format() {
            super.initDefaultFormat(this);
        }

    }
}

Please verify that this is a solution to your issue.

[soc]

@andi-huber Thanks! Is there a release/snapshot build available somewhere?

[soc]

I tried it with the following results: https://github.com/geotools/geotools/pull/3232#issuecomment-740177243

[keilw]

@andi-huber I believe all the foundations in the RI were layed already, so could this be closed before preparing a 2.1.2 version by the end of this week?

[andi-huber]

Agreed. Closing it.