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-5016] Adds support for LocalDate and adjusts tests to use the new Java 8 API #529

Merged
merged 5 commits into from
Feb 23, 2022

Conversation

lukaszlenart
Copy link
Member

Relates to WW-5016

@coveralls
Copy link

coveralls commented Feb 6, 2022

Coverage Status

Coverage increased (+0.02%) to 50.583% when pulling 59932a5 on WW-5016-uses-proper-format into f0b24d1 on master.

Copy link
Contributor

@JCgH4164838Gh792C124B5 JCgH4164838Gh792C124B5 left a comment

Choose a reason for hiding this comment

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

Hi @lukaszlenart.
The changes and updates to the unit tests seem to make sense, based on a migration from SimpleDateFormat to DateTimeFormatter. 👍
The only other consideration might be whether there would be any (or enough) value in providing a backwards-compatibility mode for the old date formatting ?
Probably not, but just raising it for consideration.

@lukaszlenart
Copy link
Member Author

Hm... I can implement a flag to either use a new or an old formatting mechanism. Yet this will introduce unnecessary complexity and I think it's a good moment to break backward compatibility while releasing a new major version.

@lukaszlenart
Copy link
Member Author

I'm working on a version which uses @Inject to inject a porper formatter.

@lukaszlenart
Copy link
Member Author

Done, I've extended the change to use to a formatter injected by the framework based on user choice.

@lukaszlenart lukaszlenart merged commit 3e65ec1 into master Feb 23, 2022
@lukaszlenart lukaszlenart deleted the WW-5016-uses-proper-format branch February 23, 2022 09:18
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.

3 participants