Human readable time of day option

[bierpub]

Create a Human Readable option for Time of Day that does not include the date that the rule started. This can be used as an alternate to the start date when wanting to print only the time of day for each occurrence and without the word starting.

Occurrences that are once per day will print the time of day
monthly on the 2nd of the month at 4:15 PM
Occurrences that are more than once per day will show the starting at time
every 7 hours starting at 6:30 AM

[rlanvin]

Hello! Thanks for the PR. It seems some tests break, could you have a look?

[rlanvin]

I left a few comments on the code, but I'd like to discuss the design of this feature. I see two main flaws in the current PR:

1. I don't like that enabling this new include_timeofday option forcibly disables include_start. It's a sign that logically this isn't a separate, independent option, but rather a variation of include_start. Therefore my first instinct is that it would make more sense to accept a 3rd value for include_start which would indicate that we want "time only" start (this could be a string value, or a constant). To preserve backward compatibility, true and false still need to work of course.
2. The new timeofday_formatter duplicates a lot of code from date_formatter, and because the two options are mutually exclusive (point 1), it's either one or the other, we'll never need both at the same time. So the duplication doesn't make sense, and I think date_formatter should be used instead of introducing a duplicated option. Actually I just realised date_formatter is also used to format the UNTIL so ok, maybe there is a case where we would need two separate formatters. It would still be good to see if the code duplication could be reduced though.

Hope that makes sense. Let me know what you think.

[rlanvin]

Think about point 1 further, another approach could be to design this as a "sub option", similar to what explicit_infinite does (it only has an effect if include_until is set). It could be something like start_time_only true/false for example, that has no effect unless include_start is set. This would probably be even better as it would be more consistent with the rest of the api.

[bierpub]

I do agree there is some duplicate code now, and I was a little hesitant doing that. Having to properly update handle multiple languages is why I ended up down this route. But I'll look back at this and see if I can make this better. I like your suggestions.

[bierpub]

Hello. I think this is much better now. There is only 1 new option start_time_only to tell it to print the time with a slightly different string. For the time formatting, it is now using the date_formatter to set the time without the date if necessary.

[bierpub]

@rlanvin Hi. Any chance we could get this reviewed and merged into the master code base? I've been using this for a few months now in our production environment and all has been good with this. Thanks.

[rlanvin]

@bierpub Yes sure - sorry I thought I already reviewed and merged this one, but I'm bad at keeping track of the open PRs at the moment... Thanks a lot for addressing my comments from January. This looks good to me.