-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
19 unit conversion #20
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way that IcatUnits
works is counter-intuitive. You have a constructor that modifies a static variable and therefore changes the behaviour of all other instances. Either have a separate non-static instance of SimpleUnitFormat
for each instance of IcatUnits
, or only have static methods and no instances.
It's not clear from the name of either class that the purpose of this is to convert quantities to SI units, so provide a method with a name such as convertToSiUnits(...)
.
I'm not totally confident on how singletons work, but since we're calling In terms of making everything static, since we're passing configuration settings in icat.server/icat.lucene I don't see a way the "converter" could be made properly static other than maybe doing
Yeah looking at it I think I was trying to persist the terminology of the underlying library (i.e. |
As you say, since this needs runtime configuration of aliases, it would be best to have it be non-static and use |
Adds support for unit conversion, to be used in icat.server and icat.lucene as part of search improvements.