flack / ranger

Formatter for date and time ranges with i18n support
73 stars 15 forks source link

handle IntlDateFormatter::NONE to return time only #1

Closed ahebrank closed 6 years ago

ahebrank commented 6 years ago

I noticed time-only output is not handled. This might be out of scope, but this PR adds a test and some conditionals to remove separators if the date_type is NONE.

flack commented 6 years ago

Hi, and thanks for the PR!

The code looks quite clean, but I wonder if it shouldn't have some more unit tests. Because the test you have only shows the case where Ranger doesn't actually do anything, i.e. the output is the same as if you would just print the two dates separately. For example, if you run with these params:

        $ranger = new Ranger('en');
        $ranger
            ->setDateType(IntlDateFormatter::NONE)
            ->setTimeType(IntlDateFormatter::MEDIUM);
        echo $ranger->format('2013-10-05 12:20:00', '2013-10-05 13:30:00');

then the algorithm kicks in and removes the duplicate PM and you get 12:20:00 – 1:30:00 PM. Is that the intended behavior, or should the two dates always be outputted separately? If it's the latter, then it might be better to just short-circuit the format logic by doing something like

if (date_format_is_none()) {
     return render_time($start) . $this->range_separator . render_time($end);
}
ahebrank commented 6 years ago

I think intended behavior would be to strip off the "PM" if its redundant since that seems to be consistent with the overall purpose. I'll add in a test for that.

flack commented 6 years ago

merged. Thx!