-
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
Added Support For Other Formats For GET_FORMAT
Function
#216
Added Support For Other Formats For GET_FORMAT
Function
#216
Conversation
Codecov Report
@@ Coverage Diff @@
## integ-add-get_format-function-formats #216 +/- ##
========================================================================
Coverage 98.36% 98.37%
- Complexity 3643 3649 +6
========================================================================
Files 343 343
Lines 9017 9057 +40
Branches 585 585
========================================================================
+ Hits 8870 8910 +40
Misses 142 142
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
.put("datetime", "usa", "%Y-%m-%d %H.%i.%s") | ||
.put("timestamp", "usa", "%Y-%m-%d %H.%i.%s") | ||
.build(); | ||
.put("date", "usa", "%m.%d.%Y") |
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.
Consider making DATE_USA and TIME_USA and then you can re-use it for datetime and tests.
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.
Consider adding "usa" as a constant to re-use in tests
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.
Consider adding "date" and "datetime", etc into a constant
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.
I've decided against doing this for now. When I try to refactor the code, it ends up looking fairly messy because many of the formats have minor differences, so it results in harder-to-read code overall. I've also decided against using the constants and sharing them with the test code because if they are shared between this class and tests, many of the tests could still pass even if the format is wrong.
Check if formats can be implemented using ones from here https://docs.oracle.com/javase/8/docs/api/java/time/format/DateTimeFormatter.html#:~:text=Predefined%20Formatters |
Signed-off-by: GabeFernandez310 <Gabriel.Fernandez@improving.com>
e6bb0e1
to
67472fc
Compare
Signed-off-by: GabeFernandez310 Gabriel.Fernandez@improving.com
Description
Adds the support for more formats for the
get_format
function to the OpenSearch SQL Plugin. Its implementation is aligned with the MySQL Documentation.Issues Resolved
opensearch-project#722
Check List
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.