Skip to content

Support iso offset time and date-time conversion with MVC and WebFlux by setting time or date-time properties to iso-offset #21630

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 8 commits into from
Closed

Support iso offset time and date-time conversion with MVC and WebFlux by setting time or date-time properties to iso-offset #21630

wants to merge 8 commits into from

Conversation

gaurav-91
Copy link
Contributor

@gaurav-91 gaurav-91 commented May 31, 2020 •
edited by philwebb
Loading

Fix for #21531

@pivotal-issuemaster
Copy link

@gaurav-91 Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 31, 2020
@philwebb philwebb changed the title Support iso offset date time format via special value for spring.mvc.format.date-time #21531 Support iso offset date time format via special value for spring.mvc.format.date-time Jun 1, 2020
@philwebb philwebb mentioned this pull request Jun 1, 2020
Closed
@pivotal-issuemaster
Copy link

@gaurav-91 Thank you for signing the Contributor License Agreement!

@gaurav-91
Copy link
Contributor Author

@philwebb: Did you get a chance to go through the PR. Please let me know if code need to be reviewed together. thanks

@philwebb
Copy link
Member

philwebb commented Jun 7, 2020

@gaurav-91 I'm afraid we have a number of bugs that need my attention before we release 2.3.1. It might be another week or so before I can get to this.

@mbhave
Copy link
Contributor

mbhave commented Jun 11, 2020

@gaurav-91 Thanks for the PR. Could you add some tests to make sure that the iso-offset pattern works as expected. You can take a look at the existing test for the "iso" pattern.

@mbhave mbhave added the status: waiting-for-feedback We need additional information before we can continue label Jun 11, 2020
@gaurav-91
Copy link
Contributor Author

Sure will add it by today.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 12, 2020
@snicoll snicoll added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Jun 12, 2020
@gaurav-91
Update WebConversionServiceTests.java
4b9fd19 
Test cases added for isooffset format
@gaurav-91
Copy link
Contributor Author

@mbhave I have added the test cases but they seem to be failing. Looks like OffsetDateTime converters are being used. And if I use LocalDate Object that fails while parsing as we are trying to parse ISO_Offset date. Could you please take a look what I am doing wrong here. Thanks

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 13, 2020
@gaurav-91
Copy link
Contributor Author

@mbhave Could you please take a look at the test cases added by me, and help me understand what I am doing wrong in that. Thanks

@mbhave mbhave added the for: team-attention An issue we'd like other members of the team to review label Jun 22, 2020
@philwebb philwebb removed the for: team-attention An issue we'd like other members of the team to review label Jun 22, 2020
@wilkinsona
Copy link
Member

Thanks for the PR, @gaurav-91.

I don't think it makes sense to offer ISO offset formatting for dates. The date format is used to configure the date format of Spring Framework's DateTimeFormatterRegistrar and the javadoc of its setDateFormatter method states that it is only used for formatting of LocalDate. A LocalDate does not contain a timezone offset so it cannot be formatted using an offset-based format as there's no offset information to format. Would you mind reworking your proposal slightly so that iso-offset is only supported for the spring.mvc.format.date-time and spring.mvc.format.time properties?

@mbhave mbhave added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Jun 22, 2020
@gaurav-91
Copy link
Contributor Author

Thank you @wilkinsona . It syncs up with my understanding, I will start working on the changes mentioned by you.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 24, 2020
@wilkinsona wilkinsona removed the status: feedback-provided Feedback has been provided label Jun 24, 2020
@wilkinsona
Copy link
Member

How's it going, @gaurav-91?

@gaurav-91
Copy link
Contributor Author

@wilkinsona I have made the changes as suggested by you. Test case for spring.mvc.format.time seems to be failing. I have committed the code done so far. Could you please take look and advice if possible. Thanks

gaurav-91 added 2 commits June 30, 2020 08:53
@gaurav-91
To fix the format Issue
2015c4d 
To fix the format Issue
@gaurav-91
IsoOffset Timeformat test case fixed
ea31215 
IsoOffset Timeformat test case fixed
@wilkinsona wilkinsona changed the title Support iso offset date time format via special value for spring.mvc.format.date-time Support iso offset time and date-time format via special value for spring.mvc.format.time and date-time Jun 30, 2020
@wilkinsona wilkinsona added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 30, 2020
@wilkinsona wilkinsona modified the milestones: 2.4.x, 2.4.0-M2 Jun 30, 2020
@wilkinsona wilkinsona changed the title Support iso offset time and date-time format via special value for spring.mvc.format.time and date-time Support iso offset time and date-time format by setting spring.mvc.format.time or date-time to iso-offset Jun 30, 2020
wilkinsona pushed a commit that referenced this pull request Jun 30, 2020
@gaurav-91 @wilkinsona
Support iso-offset for date-time and time formatting with MVC
c3b1172 
See gh-21630
wilkinsona added a commit that referenced this pull request Jun 30, 2020
@wilkinsona
Polish "Support iso-offset for date-time and time formatting with MVC"
9230ea4 
See gh-21630
@wilkinsona wilkinsona mentioned this pull request Jun 30, 2020
Closed
@wilkinsona
Copy link
Member

Thanks very much for making your first contribution to Spring Boot, @gaurav-91. The proposed changes have now been merged into master. If you're interested, I also added a small polishing commit, primarily to allow iso-offset to be used as the value as well as isooffset.

@wilkinsona wilkinsona changed the title Support iso offset time and date-time format by setting spring.mvc.format.time or date-time to iso-offset Support iso offset time and date-time format by setting time or date-time to iso-offset Jun 30, 2020
@wilkinsona wilkinsona changed the title Support iso offset time and date-time format by setting time or date-time to iso-offset Support iso offset time and date-time conversion with MVC and WebFlux by setting time or date-time properties to iso-offset Jun 30, 2020
@gaurav-91
Copy link
Contributor Author

@wilkinsona Any other issues which I could pick up.

@wilkinsona
Copy link
Member

wilkinsona commented Jul 6, 2020 •
edited
Loading

@gaurav-91 Thanks for the offer. We have an ideal for contribution label that may be of interest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wilkinsona wilkinsona wilkinsona approved these changes

Assignees

@wilkinsona wilkinsona

Labels
type: enhancement A general enhancement
Projects
None yet
Milestone
2.4.0-M2
Development

Successfully merging this pull request may close these issues.
7 participants
@gaurav-91 @pivotal-issuemaster @philwebb @mbhave @wilkinsona @snicoll @spring-projects-issues