-
Notifications
You must be signed in to change notification settings - Fork 0
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
STNG-184 Improve all DSP and related classes #198
base: dev
Are you sure you want to change the base?
Conversation
jkosternl
commented
Oct 15, 2024
- Use Jackson ObjectMapper functionality if possible
* Use Jackson ObjectMapper functionality if possible
@CodiumAI-Agent /describe |
TitleEnhance JSON Handling and Refactor Placeholder Replacements Across Modules User description
PR Typeenhancement, bug_fix, tests Description
Changes walkthrough 📝
|
return JsonToolkit.templateFileToJsonNode( | ||
"/standards/booking/messages/"+ scenarioType.bookingTemplate(apiVersion), | ||
Map.ofEntries( |
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.
Interesting, what was wrong about this way of writing it?
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.
Actually, nothing is wrong with the existing way of writing that. But since I changed the mapper, some values could be null
, which is not allowed if you use Map.entry()
. So, I favored to use the generic way of mapping objects and I needed to change this code here, to prevent failures.
Next to that, I changed JsonToolkit.templateFileToJsonNode
method, to get the same replacing behavior as before, because developers used cspNode.required("commodityType1").asText(),
way to create the Java object, which causes the String objects to get filled with "null" text.
Actually, I am now in doubt if I really need both changes. Will check if the null-check is really needed later (before merging).
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.
Personally I'd favor filtering the nulls in the JsonToolkit method, so everyone can use the fluent API without worrying about them. As for values appearing as the string "null", I'd view it as an unintended side effect that happens to work rather than as the desired behavior 🙂
replacements.forEach((key, value) -> jsonString.set(jsonString.get().replaceAll(key, value))); | ||
replacements.forEach( | ||
(key, value) -> { | ||
if (value == null) value = "null"; // Null values are replaced with "null" |
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.
Interesting, what's wrong with a "proper" null (or a null or missing node)?