-
Notifications
You must be signed in to change notification settings - Fork 20
feat(export): make lists and maps in spreadsheet export machine-readable #398
Conversation
…ble by exporting as JSON closes #353
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.
Code looks good up to minor comments, which can be solved later
.collect(Collectors.toList()); | ||
return joinStrings(mapEntriesAsStrings); | ||
Map<String, Object> originalMap = nullToEmptyMap(((Map<String, Object>) fieldValue)); | ||
Map<String, String> map = Maps.transformValues(originalMap, v -> v != null ? v.toString() : ""); |
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 function you have implemented here as lambda is already in the system as nullToEmptyString
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.
oops! That's embarrassing. I'll fix it.
try { | ||
return mapper.writeValueAsString(value); | ||
} catch (JsonProcessingException e) { | ||
throw new IllegalArgumentException(String.format("Cannot serialize field value %s to JSON", value), e); |
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.
It might be better to use no RuntimeException
here, maybe use instead SW360Exception
.
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.
Please log the exception with log.error(msg,e)
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.
You're right.
} | ||
return fieldValue.toString(); | ||
} | ||
|
||
private static String serializeToJson(Object value) { | ||
ObjectMapper mapper = new ObjectMapper(); |
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.
It might improve performance to move the ObjectMapper
to a static field
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.
It's a seldom used function. I wouldn't care about this performance gain at this point.
chore(export): clean up the code merged with PR #398 review-by:[email protected] tested-by:[email protected]
make lists and maps in spreadsheet export machine-readable by exporting as JSON
closes #353