Skip to content

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.

@lukaszlenart
Copy link
Member

@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