Skip to content

Conversation

@KochTobi
Copy link
Member

No description provided.

@KochTobi KochTobi requested a review from a team August 27, 2020 10:38
@KochTobi
Copy link
Member Author

This needs qbicsoftware/data-model-lib#52 to be merged.

Steffengreiner
Steffengreiner previously approved these changes Aug 27, 2020
Copy link
Contributor

@Steffengreiner Steffengreiner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I have some very small nitpicks, the overall work looks really clean and concise! 👍

Steffengreiner
Steffengreiner previously approved these changes Aug 27, 2020
Copy link
Contributor

@Steffengreiner Steffengreiner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@KochTobi
Copy link
Member Author

Requires qbicsoftware/parent-poms#61

@KochTobi KochTobi dismissed Steffengreiner’s stale review August 27, 2020 13:06

The code does not run until the parent-pom is fixed which can not be done until the data-model-lib is released.

@KochTobi KochTobi requested a review from a team August 31, 2020 14:01
Copy link
Contributor

@jenniferboedker jenniferboedker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! However as @KochTobi told me, the code still needs to be tested. I would strongly suggest testing the code before merging it. If I understood correctly we don't have data to test it atm.@sven1103 how do you think about it?

@KochTobi KochTobi requested a review from a team September 1, 2020 11:45
wow-such-code
wow-such-code previously approved these changes Sep 1, 2020
Copy link
Member

@wow-such-code wow-such-code left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code lgtm

@KochTobi
Copy link
Member Author

KochTobi commented Sep 3, 2020

The DTO returns null instead of empty strings whenever fields are not set in the address qbicsoftware/data-model-lib#55
Modification to the MapAnnotation generations needs to be made. Every value in the NamedValue constructors needs to be checked for null or NullPointerExceptions need to be caught for every key-value pair.

@KochTobi KochTobi dismissed wow-such-code’s stale review September 3, 2020 15:53

The current version fails for missing address attributes.

Comment on lines +1004 to +1027
try {
mapAnnotationContent.add(new NamedValue("hardware.detector.type", instrumentHardware.getDetector().getType()));
} catch (NullPointerException ignored) {}
try {
mapAnnotationContent.add(new NamedValue("hardware.objective", instrumentHardware.getObjective()));
} catch (NullPointerException ignored) {}
try {
mapAnnotationContent.add(new NamedValue("manufacturer", instrument.getManufacturer()));
} catch (NullPointerException ignored) {}
try {
mapAnnotationContent.add(new NamedValue("location.address.affiliation", instrumentLocationAddress.getAffiliation()));
} catch (NullPointerException ignored) {}
try {
mapAnnotationContent.add(new NamedValue("location.address.country", instrumentLocationAddress.getCountry()));
} catch (NullPointerException ignored) {}
try {
mapAnnotationContent.add(new NamedValue("location.address.street", instrumentLocationAddress.getStreet()));
} catch (NullPointerException ignored) {}
try {
mapAnnotationContent.add(new NamedValue("location.address.zipCode", instrumentLocationAddress.getZipCode().toString()));
} catch (NullPointerException ignored) {}
try {
mapAnnotationContent.add(new NamedValue("location.roomId", instrumentLocation.getRoomId()));
} catch (NullPointerException ignored) {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: the underlying problem is, that the instrument properties can be null. So lets make sure they are never null, but empty Strings, Lists or empty objects.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to be addressed in the instrument constructor then.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created an issue in the data-model-lib qbicsoftware/data-model-lib#55

@KochTobi
Copy link
Member Author

@luiskuhn @wow-such-code Is this still needed? if not, feel free to close this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants