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

WW-5530 make DateConverter work for LocalDate and LocalTime #1223

Merged
merged 4 commits into from
Feb 26, 2025

Conversation

bill-humblcloud
Copy link
Contributor

@bill-humblcloud bill-humblcloud commented Feb 17, 2025

WW-5530

I found that the DateConverter class could not convert for java.time.LocalDate or java.time.LocalTime input—only java.time.LocalDateTime worked. The reason for this is that these types were not handled in the XWorkBasicConverter class; thus, they could not be properly directed to the DateConverter class. This simple patch fixes that deficiency. [This is my first pull request, so sorry if I've done anything wrong. I believe the online docs are somewhat out of date. ] Thanks.

@lukaszlenart
Copy link
Member

Thanks for the PR. Could you create a ticket in JIRA which will relate to this change? Thanks in advance!
https://issues.apache.org/jira/projects/WW

@lukaszlenart lukaszlenart added this to the 7.1.0 milestone Feb 19, 2025
@lukaszlenart lukaszlenart changed the title make DateConverter work for LocalDate and LocalTime WW-5530 make DateConverter work for LocalDate and LocalTime Feb 19, 2025
@lukaszlenart
Copy link
Member

Great, could you extend XWorkConverterTest as well? There are already some test cases addressing conversion of dates.

@lukaszlenart
Copy link
Member

Great, could you extend XWorkConverterTest as well? There are already some test cases addressing conversion of dates.

@bill-humblcloud do you have spare cycles to address this comment?

@bill-humblcloud
Copy link
Contributor Author

I have modified XWorkConverterTest.testDateConversion() to include tests for LocalDate, LocalDateTime, and LocalTime. The tests seem to pass during the build process. I am new to GitHub, so I am working to figure out how to get those changes uploaded here (should've made notes the first time I did this). I will try to get that done in the next 24 hours (simultaneously working on a release of my own project, so I will do my best).

@lukaszlenart
Copy link
Member

You can edit the file in your fork here even directly via the web editor on Github, commit the changes and they should appear in the PR.

@bill-humblcloud
Copy link
Contributor Author

I believe the pull request now looks as it should with the test changes included. Please let me know if you see things differently. Thanks for the help!

@lukaszlenart
Copy link
Member

Looks good

@lukaszlenart lukaszlenart merged commit 0d2d110 into apache:main Feb 26, 2025
6 checks passed
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.

2 participants