Retrieve prefixes by class instead of string in SystemOfUnitsService about unit-api HOT 15 CLOSED

Not sure why we need to specify a class? If the SystemOfUnitsService interface is implemented by libraries, it seems to me that those libraries known their implementation class?

If for some reason we really want to keep the Class argument, then at the very least it shall be Class<?> (or more specific type is we know it) and the @SuppressWarnings("rawtypes") should be removed.

from unit-api.

Because it is the only way it'll work with Prefix implementations that are an Enum. The way the JVM does this at runtime you have no <?> argument to use.

Collections.<Prefix> unmodifiableSet(EnumSet.allOf(prefixType))

in
DefaultSystemOfUnitsService shows how it works. Happy about suggestions what to do if it's not an enum, e.g. throw an exception, but you can't use Class<Prefix> or Class<P extends Prefix> not even Class<?> will work, try it, the compiler fails with some weird generic exception.

from unit-api.

It is not the only way it will work. The clean way to work with Class<?> prefixType is:

EnumSet.allOf(prefixType.asSubclass(Enum.class))

But even if for some reason you wanted raw types, keep it in the implementation, not in the API. You can have Class<?> in the public interface and only Class in the implementing method (because the JVM does type erasure as you mentioned).

from unit-api.

However the above does not answer the question about why we need a Class<?> argument in the first place.

from unit-api.

What is there will not work with Enum.class that isn't specific enough, you pass e.g.

EnumSet.allOf(MetricPrefix.class))

And the Class instead of a String as argument avoids boilerplate code or some instanciation.

If the AsSubclass option with a generic wildcard would work and compile in the RI, please push or PR it there.

from unit-api.

Werner, it will work with Enum.class. Read the Javadoc.

from unit-api.

If you have time to propose a PR or push it directly, please feel free to do. I am not on my own computer but maybe tonight or on the weekend I could also do.

If trying to apply asSubclass(Enum.class) would also throw an appropriate exception (ClassCastException or similar) when you try to pass say Number.class instead of MetricPrefix.class then it would even make it a one-liner ;-)

from unit-api.

Indeed, prefixType.asSubclass(Enum.class) throws ClassCastException if prefixType is not a subtype of Enum. I will try to create pull requests soon.

from unit-api.

Merged everything. Thanks @desruisseaux, I also removed the extra if/else check and added a JUnit test for ClassCastException. I added author references and mentioned it may throw a ClassCastException in the API. Note, it is implementation-specific. A different implementation may also chose a Map of Sets with the Class as a key to retrieve the content of a Prefix implementation. Using enum keeps it flexible and it works even for unknown implementations (in fact it should work even for those declared by another implementation here as long as it also relies on enum ;-) but it is not a requirement of the Spec or API that enums must be used.

from unit-api.

Thanks. But it still not clear to me how the Class<?> argument achieve implementation independence? In the way it is currently done, isn't the user have to know the Enum type (s)he is using, which is specific to every implementation?

from unit-api.

The class has to be from a particular implementation, but at least those who use enums the implementation does not even have to be Indriya, it could also be UOMo or SIS (should Prefix even be relevant there)

from unit-api.

But still, the user can not said "give me the SI prefix of whatever implementation is behind that SystemOfUnitsService implementation". (S)he become tied to a particular implementation.

For a library for example, it become impossible to let the user choose his JSR 385 implementation and get the SI prefixes of that implementation. The library would have to said for example getPrefixes(TheIndriyaImplementationOfPrefix.class), which force the library users to have at least that particular implementation on his classpath (even if (s)he uses another one for other operations).

from unit-api.

Unlike SystemOfUnits etc. which have stronger ties to abstract base classes if Enum is the only common ground, it would be able to retrieve TheUOMoImplementationOfPrefix as long as you know the class and it's on the classpath/module path. What may be an issue however are the UnitConverter implementations, but leaving those aside it is slighly more flexible than other services.

Would you prefer to use a string approach even with possible aliases like we do to retrieve unit systems?
In that case caching only the known sets of prefixes instead of retrieving them dynamically would be the only way I can see. Because for enums everything is pretty static and does not change at runtime it would give the same results, but caching the sets in a Map<String, Set> like structure would be an overhead.

This would also require something like getAvailablePrefixCollections (I honestly don't know the name here, but PrefixDB from UCAR comes close) telling you the ids or aliases of those prefix DBs. By using the class it seems unnecessary here, but if there was such a strong desire to cache it and use string IDs we could do that.

from unit-api.

I understand that any implementation based on Class.getEnumConstants() can work with any Class<? extends Enum>, but a user who want to use this API still have to know the TheUOMoImplementationOfPrefix class. He has to tie himself to a particular implementation.

A String argument would avoid this problem, provided that we standardize some String values (e.g. "SI"). I would be fine with other type of argument too, as long as they allow the use of the API in an implementation-independent way. Current use of the Class argument does not allow that (even if Indriya can get "foreigner" Enum, the user is still tied to a particular implementation).

from unit-api.

It's actually a little more complicated than simply chosing class or string. I created #85, because the current approach in any case forces EVERY module to override that method even if it has no use for prefixes. E.g. SI uses the same prefixes we declared in the RI already, In theory if another implementation decides to only go with BinaryPrefix or another prefix it could declare them in another module. MetricPrefix is so common it makes sense in the RI. So we have to decide if we go for a simple one-liner possibly even as default method in the API works for most users (it still allows to override for every implementation that needs to store it a different way) or add the same boilerplate we do for unit systems, but unless we decided to introduce a totall new PrefixService just for the prefix, it must be implemented in an abstract way so all systems can avoid it and reuse from the implementation if they do not need their own prefix.

from unit-api.