Issue unparsing duration

[zalky]

Running into an issue with `cljs-time.format/unparse-duration:

cljs.user> (def prev (t/now))
#'cljs.user/prev
cljs.user> (tf/unparse-duration (t/->period (t/interval prev (t/now))))
#error {:message "Months cannot be converted to millis", :data {:type :unsupported-operation}}
Error: Months cannot be converted to millis
    at new cljs$core$ExceptionInfo (http://localhost:4000/js/main.out/cljs/core.js:35447:10)
    at Function.cljs.core.ex_info.cljs$core$IFn$_invoke$arity$3 (http://localhost:4000/js/main.out/cljs/core.js:35508:9)
    at Function.cljs.core.ex_info.cljs$core$IFn$_invoke$arity$2 (http://localhost:4000/js/main.out/cljs/core.js:35504:26)
    at cljs$core$ex_info (http://localhost:4000/js/main.out/cljs/core.js:35490:26)
    at cljs_time$core$conversion_error (http://localhost:4000/js/main.out/cljs_time/core.js:2406:25)
    at cljs_time.core.Period.cljs_time$core$InTimeUnitProtocol$in_millis$arity$1 (http://localhost:4000/js/main.out/cljs_time/core.js:2432:40)
    at cljs_time$core$in_millis (http://localhost:4000/js/main.out/cljs_time/core.js:419:14)
    at cljs_time$format$unparse_duration (http://localhost:4000/js/main.out/cljs_time/format.js:824:59)
    at eval (eval at <anonymous> (http://localhost:4000/js/main.out/weasel/repl.js:30:495), <anonymous>:1:114)
    at eval (eval at <anonymous> (http://localhost:4000/js/main.out/weasel/repl.js:30:495), <anonymous>:9:3)

Using Clojure 1.9.0-beta4, Clojurescript 1.9.946. Am I doing something wrong?

[shen-tian]

Looking at this line in format.cljs. unparse-duration converts a period to milliseconds first, then to the duration string. However, you can only do this for periods of less than a month (since the length of a month is variable).

Potential fix would be to relax the conversion of periods to millis here to allow for cases with 0 month and years. But this still doesn't fix the general case.

Looking at what unparse-duration does, it converts Intervals and Periods to xxx days xx hours xx minutes using goog.date.duration.format, which cannot be done in a well specified manner for a period such as "2 Month".

So, the consistent solution is actually to say that Periods can't be unparsed this way. But probably time to ask lib author @andrewmcveigh ...

[andrewmcveigh]

I think probably the best thing to do is to not set :months & :years at all here (when they are zero) https://github.com/andrewmcveigh/cljs-time/blob/c6c304745ea9f9b0201f295ca97d1532e6ee1f4b/src/cljs_time/core.cljs#L804

I think I'd say that's a bug.

[zalky]

Out of curiosity, what benefit does the conversion + goog.date.duration/format provide over a naive implementation (iterate over record without conversion)?

[andrewmcveigh]

As far as I remember, none. Other than I didn't have to write the conversion or goog.date.duration/format.

[shen-tian]

So, what the right behaviour then? clj-time doesn't have an unparse-duration. We have:

(time-format/unparse-duration (time/map->Period {:days 1})) => "1 day"

what should

(time-format/unparse-duration (time/map->Period {:years 1})) => ??

feels like it should either be "1 year", in which case we should skip goog.date.duration/format, or it shouldn't work at all for Periods in general.

[andrewmcveigh]

I think your intuition is right, it should be "1 year", and "2 years", "1 month", "2 months", etc.

But, it's a bit tricky in that cljs-time's periods are a bit loose (as are org.joda.time.Periods) in that you can have a period like:

{:days 200 :years 1 :months 80}

What even is that? Should we care?

[zalky]

Given that any arbitrary period only makes sense contextually (because of the conversion issue), I don't think the formatter can worry about it.

The formatter should probably just worry about generating a string from the period in a naive way. It's probably best to leave it to the caller to understand the contextual nature of a period, and whether the period they are dealing with is in a fully reduced form.

This naive formatting function would still have utility and save folks from having to re-implement it.

IMO, the place to worry about whether a period is automatically reduced is probably in cljs-time.core/->period.

[andrewmcveigh]

Yep, I think you're right. Was about to say the same.