Added a new parse util method to avoid repetition / refactored code with new method

[martinpkr]

Description

This change adds a static util method that helps with reusing a piece of code and avoids repetition for the future

Issues Resolved

Will resolve [FEATURE] Create a new Util method for a common pattern -> #720

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[dbwiddis]

Thanks for your contribution! This looks great!

Note there's 2 more changes upcoming in #718, so if that gets merged first you can update, or if your PR gets merged first than @owaiskazi19 can update. :)

Can you make a few naming changes to make the method more generic? Also we need to add a CHANGELOG entry and add a unit test in ParseUtilsTest?

[martinpkr]

Yes, i will get on it !

…

On Thu, 30 May 2024 at 18:50, Daniel Widdis ***@***.***> wrote: ***@***.**** commented on this pull request. Thanks for your contribution! This looks great! Note there's 2 more changes upcoming in #718 <#718>, so if that gets merged first you can update, or if your PR gets merged first than @owaiskazi19 <https://github.com/owaiskazi19> can update. :) Can you make a few naming changes to make the method more generic? Also we need to add a CHANGELOG entry and add a unit test in ParseUtilsTest? ------------------------------ In src/main/java/org/opensearch/flowframework/util/ParseUtils.java <#721 (comment)> : > @@ -472,4 +473,8 @@ public static Map<String, String> convertStringToObjectMapToStringToStringMap(Ma return stringToStringMap; } } + + public static Boolean checkIfInputsContainsKey(Map<String, Object> inputs, String enumKey){ This method is good but the naming implies we're just checking whether the key exists. How about parseBooleanIfExists or something similar? Also as a more generic util method it may not be "inputsso just a more generic map variable name; and the key may be a string constant, not necessarily an enum so justkey` probably works there. This needs a Javadoc added explaining what the method does. — Reply to this email directly, view it on GitHub <#721 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AU52WT3KIH4TJ2IVMRBZPBDZE5DD3AVCNFSM6AAAAABIQ4AWA6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAOBYG4YDCMBSHA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[martinpkr]

this is a good suggestion! But to be right instead of + return Float.parseFloat(value.toString()); it should be + return type.cast(Float.parseFloat(value.toString())); i will push the changes now!

…

On Sat, Jun 1, 2024 at 1:49 AM Owais Kazi ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/main/java/org/opensearch/flowframework/util/ParseUtils.java <#721 (comment)> : > + /** + * Checks if the inputs map contains the specified key and parses the associated value to a generic class. + * + * @param inputs the map containing the input data + * @param key the key to check in the map + * @param type the class to parse the value to + * @return the generic type value associated with the key if present, or null if the key is not found + */ + public static <T> T parseIfExists(Map<String, Object> inputs, String key, Class<T> type) { + if (!inputs.containsKey(key)) { + return null; + } + + Object value = inputs.get(key); + if (type == Boolean.class) { + return type.cast(Boolean.valueOf(value.toString())); ⬇️ Suggested change - return type.cast(Boolean.valueOf(value.toString())); + return Booleans.parseBoolean(value.toString()) ------------------------------ In src/main/java/org/opensearch/flowframework/util/ParseUtils.java <#721 (comment)> : > + * + * @param inputs the map containing the input data + * @param key the key to check in the map + * @param type the class to parse the value to + * @return the generic type value associated with the key if present, or null if the key is not found + */ + public static <T> T parseIfExists(Map<String, Object> inputs, String key, Class<T> type) { + if (!inputs.containsKey(key)) { + return null; + } + + Object value = inputs.get(key); + if (type == Boolean.class) { + return type.cast(Boolean.valueOf(value.toString())); + } else if (type == Float.class) { + return type.cast(Float.valueOf(value.toString())); ⬇️ Suggested change - return type.cast(Float.valueOf(value.toString())); + return Float.parseFloat(value.toString()); — Reply to this email directly, view it on GitHub <#721 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AU52WT2W4Z2DM4BKW7PNHMLZFD46RAVCNFSM6AAAAABIQ4AWA6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAOJRHAYDCOJWGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[owaiskazi19]

@martinpkr you also need to amend your changes with -s for the DCO check to pass.
git commit -s --amend and also need to run ./gradlew spotlessApply to format the changes

[dbwiddis]

@martinpkr thanks for your patience working through code review! one note: you do need to sign your commits with DCO (Signed-off-by: Your Name [email protected]). If committing on git command line just use -s assuming you've configured your name and email in git.

Also be sure to run ./gradlew spotlessApply and add a change log entry!

[owaiskazi19]

add a change log entry!

@dbwiddis since this PR just adds a util method, we can ignore the CHANGELOG. Wdyt?
@martinpkr javadoc check is failing with the below

Error: eckstyle] [ERROR] /home/runner/work/flow-framework/flow-framework/src/main/java/org/opensearch/flowframework/util/ParseUtils.java:485:20: Expected @param tag for '<T>'. [JavadocMethod]

[martinpkr]

Signed off my commits and did an changelog entry for the issue,also fixed the javadoc to include info about , tell me if there is anything else .

[owaiskazi19]

Signed off my commits and did an changelog entry for the issue,also fixed the javadoc to include info about , tell me if there is anything else .

Just ./gradlew :spotlessApply is left and we should be good to merge this.

[martinpkr]

Signed off my commits and did an changelog entry for the issue,also fixed the javadoc to include info about , tell me if there is anything else .

Just ./gradlew :spotlessApply is left and we should be good to merge this.

just done that , see if everything is okay now... first time using gradle, so i'm sorry for the confusion.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.41%. Comparing base (d027c74) to head (31d3ff3).

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #721      +/-   ##
============================================
+ Coverage     74.33%   74.41%   +0.07%     
- Complexity      742      744       +2     
============================================
  Files            84       84              
  Lines          3807     3811       +4     
  Branches        332      330       -2     
============================================
+ Hits           2830     2836       +6     
  Misses          823      823              
+ Partials        154      152       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[owaiskazi19]

All the checks passed. Thanks @martinpkr for addressing the comments patiently. Welcome to the club and feel free to pick up other good first issues as well.

[dbwiddis]

LGTM!

Thanks @martinpkr for your patience here and expanding this PR to be much more useful than we originally envisioned!