I just added some unit tests to
OpenmrsUtil.join which is used in core only to join a list/set of modules to a string so it can be shown in a error message.
For example in
MandatoryModuleException we use it to compose error message
"The following modules are marked as 'mandatory' but were unable to start: "
+ OpenmrsUtil.join(moduleIds, ",")
I thought of replacing our logic in
StringUtils.join which does almost exactly the same, except for two edge cases:
found two edge cases, were transform to (string on the right is the result):
- if an element in the list is null in a list like
- if you choose a separator of null and have a list of
"module1", "module2" -->
StringUtils.join replaces a null separator or null elements with an empty string, which would turn to
- “module1”,null,“module2” --> “module1,module2”
- if you choose a separator of null and have a list of “module1”, “module2” --> “module1module2”
Does that change make sense to you?
OpenmrsUtil.join doesn’t make much sense. Better use Java 8’s in-built
String.join(delimiter, elements) though.
thanks for the tip @gayanw! Java’s built-in would still behave the same as described in edge case 1
Note that if an individual element is null, then "null" is added.
not sure if we want that or not.
Am happy to hear more opinions.
If the edge case is not important (it doesn’t break compatibility) I’d vote to deprecate it and use String.join
Ohh I see. It would be even better to wrap it within OpenmrsUtil.join in case we want to make future changes without much penalty. And to provide single interface for all native OpenMRS code.
I probably wrote OpenmrsUtil.join…sorry!
I expect this is now used by various modules, so you’ll want to deprecate it and leave around an implementation that delegates to some other library.
I feel that it’s okay to change the null behavior. (Personally I would not expect to have a “convenience” removeNulls happen, but I don’t think it’s important.)
no need to be sorry, it does work, and now there are other solutions
The issue is here
I deprecated it, but didnt change the implementation because I don’t think it is really necessary. I added tests for OpenmrsUtil.join before and it works the way it works for some time already. Now with the deprecation notice/hint developers can use String.join from java or StringUtils.join if needed instead.