search

ocpsoft / prettytime

Social Style Date and Time Formatting for Java
http://ocpsoft.org/prettytime/
Apache License 2.0
1.29k stars 252 forks source link

Add support for the ChronoUnit enum. #249

Closed Isira-Seneviratne closed 1 year ago

Isira-Seneviratne commented 1 year ago

Refactors the code to make use of java.time's ChronoUnit enum. (Issue #247)

lincolnthree commented 1 year ago

Hi @Isira-Seneviratne - Thank you VERY MUCH for all of the work you have put into this PR. I think it is moving in the right direction, but there are some things that need to be changed. I am grateful for all of your effort and I think we can make this a great update to PrettyTime!

A few comments to summarize the review above:

  1. Please use individual imports instead of wildcards *. Please retain the order of import statements.
  2. ResourcesChronoUnit should be refactored into a static utility. TimeUnit exists for the reason that it is intentionally more flexible than what an enum can provide. The java.time.temporal.ChronoUnit type should simply be added as a configuration option in org.ocpsoft.prettytime.PrettyTime configuration methods.
  3. Most of the changes in the Java Resource translation bundles can be reverted. There are some that I agree with, however. Please see my comments there.
  4. Some test cases were deleted. Could you explain why?
  5. You've used ResourcesChronoUnit in a lot of internal places. Those will need to be reverted to use TimeUnit. We can discuss additional API changes to reduce dependency on the actual Java Class implementations of TimeUnit in a separate PR!

Again, thank you so much for this. Please review my comments on the PR (in the review), and I look forward to getting this into the library!

Isira-Seneviratne commented 1 year ago

Hi @Isira-Seneviratne - Thank you VERY MUCH for all of the work you have put into this PR. I think it is moving in the right direction, but there are some things that need to be changed. I am grateful for all of your effort and I think we can make this a great update to PrettyTime!

A few comments to summarize the review above:

  1. Please use individual imports instead of wildcards *. Please retain the order of import statements.
  2. ResourcesChronoUnit should be refactored into a static utility. TimeUnit exists for the reason that it is intentionally more flexible than what an enum can provide. The java.time.temporal.ChronoUnit type should simply be added as a configuration option in org.ocpsoft.prettytime.PrettyTime configuration methods.
  3. Most of the changes in the Java Resource translation bundles can be reverted. There are some that I agree with, however. Please see my comments there.
  4. Some test cases were deleted. Could you explain why?

Due to using the new enum I added, those tests were failing. That shouldn't be an issue since I'll be updating the changes based on your comments, so I'll restore the tests.

  1. You've used ResourcesChronoUnit in a lot of internal places. Those will need to be reverted to use TimeUnit. We can discuss additional API changes to reduce dependency on the actual Java Class implementations of TimeUnit in a separate PR!

Again, thank you so much for this. Please review my comments on the PR (in the review), and I look forward to getting this into the library!

Of course.

lincolnthree commented 1 year ago

Sorry for the delay @Isira-Seneviratne - The holidays here are a busy time! I will get this reviewed again and hopefully merge ASAP!

lincolnthree commented 1 year ago

Apologies again for the delay. I am putting this on my list to complete this week.