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

Use CopyOnWriteArrayList in PrettyTime. #256

Open Isira-Seneviratne opened 1 year ago

lincolnthree commented 1 year ago

Hey @Isira-Seneviratne - Are you working on something new? :) Anything I should be aware of?

Isira-Seneviratne commented 1 year ago

Hey @Isira-Seneviratne - Are you working on something new? :) Anything I should be aware of?

Yeah, I used ConcurrentSkipListSet in this PR instead of a volatile list.

lincolnthree commented 1 year ago

Interesting. Can you tell me the reasons you went with this List implementation? I can guess, but I just want to make sure :)

Isira-Seneviratne commented 1 year ago

Interesting. Can you tell me the reasons you went with this List implementation? I can guess, but I just want to make sure :)

It's actually a Set implementation, but it's designed for concurrent access, so a volatile variable is not needed.

Also, IntelliJ recommended using a thread-safe type instead of a volatile field.

lincolnthree commented 1 year ago

Thanks, that helps me understand. There isn't technically anything wrong with volatile here. But I'm sure IntelliJ is scaring people away from it because it's not commonly used, and can be easy to get wrong by accessing non-threadsafe collections or objects across threads, or using the keyword when not necessary.

I would consider this...

Unfortunately, however, this PR breaks the tests (the ones that are passing) for TimeUnit configuration: https://github.com/ocpsoft/prettytime/blob/master/core/src/test/java/org/ocpsoft/prettytime/PrettyTimeUnitConfigurationTest.java

Note that order of TimeUnits matters, and the default configuration does something a bit strange, providing the "JustNow" TimeUnit: https://github.com/ocpsoft/prettytime/blob/master/core/src/main/java/org/ocpsoft/prettytime/PrettyTime.java#L1647

This TimeUnit is responsible for the default behavior (overriding Milliseconds and Seconds) by default so that anything less than a minute ago/from now shows 'just now' - a more human readable time measure for "short durations".

With your PR, that unit is sorted to somewhere near Minute, and loses priority to Milliseconds and Seconds when calculating non-precise durations - and that's not what the default behavior should be, since the unit needs to be in first place.

If you can figure out a way to do this without breaking the order of the default units, then let's do it. To be honest -- it's been a while since I coded that part of PrettyTime, so it might be worth re-visiting how unit order is preserved and set anyway.

lincolnthree commented 9 months ago

Sorry for the delay on this @Isira-Seneviratne - I will work on it ASAP. Life has gotten a bit unpredictable here.