Skip to content
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

Add support for SnakeYaml to be able to parse timestamps correctly when timezone is set or not #5626

Merged
merged 7 commits into from Mar 12, 2024

Conversation

MalloD12
Copy link
Contributor

Impact

  • Bug fix (non-breaking change which fixes expected existing functionality)
  • Enhancement/New feature (adds functionality without impacting existing logic)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

A new ConstructYamlTimestamp class was created to identify whether a date time should be returned with the timezone included or not.

Fixes #5499

Things to be aware of

  • None

Things to worry about

  • None

Additional Context

N/A

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you point out what part of this class is different from the class it is extending?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, the updated/added parts from the original code are these two if blocks:

if (timezoneh_s != null) {
                String time = timezonem_s != null ? ":" + timezonem_s : "00";
                timeZone = TimeZone.getTimeZone("GMT" + timezoneh_s + time);
                calendar = Calendar.getInstance(timeZone);
                ;
            } else {
                calendar = Calendar.getInstance();
            }

and

 if (timeZone == null) {
                return new java.text.SimpleDateFormat("yyyy-MM-dd HH:mm:ss").format(calendar.getTime());
            } else {
                return calendar.getTime();
            }

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a change that would be beneficial to make to SnakeYaml itself? I think that if possible, that would be more preferable. I am concerned that as the SnakeYaml class we're extending changes, we won't get the changes ourselves because we override so much of that class.

Copy link
Contributor Author

@MalloD12 MalloD12 Feb 23, 2024

Choose a reason for hiding this comment

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

Steve, do you mean we should create an issue or request the SnakeYaml team to make this change we have done here? I think it's true if they make any sort of change to the ConstructYamlTimestamp we won't get it as we override it, although I'm not sure if that could be any time soon. If you guys agree I can create an issue for this change on their end, but let's move ahead with this change and once they fix it on their end we can revert this change. How does it sound to you?

cc: @filipelautert

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds reasonable. I was suggesting that we make a PR on their repo, not an issue, since we know what the change should be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not able to create a PR on their repo, but I created this issue for them with the proposed changes we added in this PR. In the meantime, as mentioned above I think we can move forward with this PR and revert these changes once they apply the fix for it (if they do).

Thanks,
Daniel.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks Daniel!!

@MalloD12
Copy link
Contributor Author

Hey @filipelautert,

This is pretty much all snakeyaml code. I don't think it's worth spending time on it, since we are going to revert/update this code soon (when they get their code updated).

Thanks,
Daniel.

Copy link
Contributor

@rberezen rberezen left a comment

Choose a reason for hiding this comment

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

Test failure is expected and will be fixed after sync with master. Tested manually, LGTM

@filipelautert filipelautert merged commit 74183f5 into master Mar 12, 2024
32 of 34 checks passed
@filipelautert filipelautert deleted the fix-issue-5499 branch March 12, 2024 11:21
@filipelautert filipelautert added this to the 1NEXT milestone Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
4 participants