luxon icon indicating copy to clipboard operation
luxon copied to clipboard

Formatting of negative durations contains extra minus chars

Open fruitraccoon opened this issue 5 years ago • 10 comments

When toFormat("hh:mm") is used on a negative Duration, the resulting string can have extra minus characters.

Example:

const x = Duration.fromObject({ minutes: 60 });
const y = Duration.fromObject({ minutes: 150 });
const diff = x.minus(y);
diff.toFormat("hh:mm"); // Outputs "-1:-30" rather than the expected "-1:30"

Example is available here: https://codesandbox.io/s/43wry09vw4

Version 1.10.0

fruitraccoon avatar Jan 25 '19 05:01 fruitraccoon

Yeah, the formatter is a little dumb about negatives. It's saying "(-1):(-30)" because it doesn't know how to pull the negative out as "-(1:30)"

icambron avatar Jan 25 '19 06:01 icambron

@icambron Can I take this issue? So In this example I just need to print in "-(1:30)" format, right, when user use toFormat("hh:mm") and pass hh:mm as parameter ?

chinmaydalvi avatar Oct 08 '19 07:10 chinmaydalvi

@chinmaydalvi I think there's some thinking required here, because it's a little under-specified. There are a few issues to sort out:

  1. Units can be mixed, with some negative and some positive. How do we handle that?
  2. Do we really want parens? I put them in to illustrate, but I'm not sure if they're really the best thing to show, since they aren't in the format string.
  3. Is this worth fixing, or should we just say "if you want nuanced negative handling, you're on your own"?

I welcome everyone's thoughts on this, but frankly I'm tempted toward 3.

icambron avatar Oct 08 '19 20:10 icambron

I had a bit of a think about it and a bit of a look at the code. Formatting certainly does not seem like the easiest thing to change.

To address the comments from @icambron above:

  1. Units can be mixed, with some negative and some positive. How do we handle that?

We really just need to know if the total of the duration is less than 0 - if it is, then a single - can just be added "somewhere". But should it be to the left of the left-most value in the format string? Perhaps there needs to be a format string symbol so that it can be placed explicitly?

Maybe there's some guidance that can be taken from the standard libraries of other languages in how they handle formatting negative durations.

  1. Do we really want parens? I put them in to illustrate, but I'm not sure if they're really the best thing to show, since they aren't in the format string.

No, I agree that we'd only want to show characters that are in the format string.

  1. Is this worth fixing, or should we just say "if you want nuanced negative handling, you're on your own"?

It would be good to get it fixed if possible. That said, the following is a workaround:

const testValue = Duration.fromObject({ days: -1, minutes: 59 });
const testValueMs = testValue.as('milliseconds');
const isNeg = testValueMs < 0;
const positiveTestValue = Duration.fromMillis(Math.abs(testValueMs));
const formatted = positiveTestValue.toFormat(`${isNeg ? '-' : ''}hh:mm`);
// formatted: "-23:01"

fruitraccoon avatar Oct 11 '19 02:10 fruitraccoon

My solution:

function isNegativeDuration (duration) {
  return duration.valueOf() < 0
}
function formatDuration (duration, pattern) {
  if (isNegativeDuration(duration)) {
    return `-${duration.negate().toFormat(pattern)}`
  } else {
    return duration.toFormat(pattern)
  }
}
formatDuration(Duration.fromObject({ minutes: 1 }), 'h:mm')  // 0:01
formatDuration(Duration.fromObject({ minutes: -1 }), 'h:mm')  // -0:01

munierujp avatar Dec 01 '19 13:12 munierujp

On mixed-sign values:

We really just need to know if the total of the duration is less than 0 - if it is, then a single - can just be added "somewhere".

This is not that simple. The first problem is that finding out it's negative requires converting between units. That's no big deal as long as we respect the conversionAccuracy setting (which toMillis does), but it's a little strange. The second problem is to be useful, we need all the units to have the same sign, whichever one we decided the total is. This should be the job of normalize, but it currently does not always do that:

> Duration.fromObject({ months: -1, days: 30, hours: -5}).normalize().toObject()
{ months: 0, days: -1, hours: 19 }

So I think we need to fix normalize to take care of that. Then we just check if the toMillis() < 0 and if so, normalize.

But should it be to the left of the left-most value in the format string? Perhaps there needs to be a format string symbol so that it can be placed explicitly?

I'd prefer we just make a convention for it, like -([your format])

icambron avatar Dec 10 '19 08:12 icambron

you can apply normalize until nothing is to be normalized anymore:

  let normalized = value.normalize();
  while (!normalized.equals(value)) {
    value = normalized;
    normalized = value.normalize();
  }

all components end up being positive or negative, not pretty but it does the job

backbone87 avatar Apr 21 '20 00:04 backbone87

  1. Units can be mixed, with some negative and some positive. How do we handle that?

I am not sure what is the use-case of mixing units with different signs, I am sure there are still. Do you have an example? In moment.js this has been discussed (https://github.com/moment/moment/issues/2408) but it's not part of any spec if I understood well.

What if we just threw errors if users try to provide units with different signs?

As for how I encountered this error, I tried to do:

Duration.fromObject({minutes: -200}).forFormat("hh:mm");

And the output was: -3:-20 instead of my own expectation of -03:20 (just like GMT offsets).

vvo avatar Jul 09 '20 08:07 vvo

TL;DR: no heuristics in a library, please!

I know this is an old issue (and a tough one), but I thought it might be interesting to share my experience with durations. I have worked in the insurance industry for 25 years now, where date arithmetic is core business. Insurance policies need all sorts of date/duration calculations that compute future/past effective dates of several events like expiry, premium due dates, etc. These use different duration units (years, quarters, months and days are the most frequent ones). Most systems have very (i.e. think insanely) sophisticated logic - mostly coded as a mess :) , btw.

I am not sure what is the use-case of mixing units with different signs, I am sure there are still.

Yes, there are. One very simple example would be "calculate the last day of a policy year starting on day x", which corresponds to a date / duration pseudo-expression of x + (+1 year -1 day). Other, much more complex scenarios also exist. Mixing positive and negative components in a duration is definitely a valuable feature of a library.

My experience can be summed up as

  • Whatever you can theoretically/conceptually imagine, will sooner or later have an actual use case;
  • Date/duration arithmetic is much more complex in practice than anyone would imagine first;
  • Duration arithmetic is never completely unambiguous and cannot be handled with one solution that is complete, intuitive and consistent at the same time (hence the code mess in the systems);
  • Anecdotally analyzing/planning a certain feature always leaves a large part of the possible scenarios completely undetected;
  • (Therefore:) general date/duration libraries are best if the results produced are predictable and simple. Simplicity means being as free from hidden heuristics as possible. Exceptional cases will always emerge but should never be coded in the library because (1) the unwanted impact on unexpected or yet undiscovered scenarios is huge and (2) in the end such things lead to losing trust: programmer eventually codes all specific scenarios from scratch to make sure (or feel sure?) that it does what is intended. Believe me, I have seen insane code that avoids using library functionality and goes back to manipulating dates and durations on the component level quite a lot.

My personal analysis/opinion/preference on the original request:

Note the formatting option being used: .toFormat("hh:mm"). This says nothing about sign. Nevertheless, hiding the possible sign in the produced output is obviously not a good choice.

This example is special in the sense that minutes / hours can always be exactly converted. It is much better to think about units that cannot be converted consistently (like year / day, month / day, etc.). For example all of the following are meaningful and possible durations: -1 year -1 day, -1year +1 day, +1 year -1 day, +1 year +1 day. Out of those, the request would apply to -1 year -1 day

To me it seems arbitrary (and unwanted) to have some "hidden heuristic" that results in a "magic sign" jumping somewhere (where exactly?) for one specific case only, especially if we consider the possible set of format strings.


BTW (somewhat related to this issue): to me even the fact that .toFormat() does converting and normalization behind the scenes is unwanted. This is precisely the kind of heuristic which ultimately makes programmers start recoding behavior as part of the business logic. For example:

Duration.fromObject({year:1, day:-1}).toFormat("y d") // "0 364"

Such behavior results in loss of information. The internal Duration object is +1 year -1 day, not 0 year +364 day. Also, it is sometimes inaccurate as 1 (calendar !) year is either 365 or 366 days long.

balagge avatar Sep 17 '20 10:09 balagge

> Duration.fromISO('PT1M1S').negate().toString() 'PT-1M-1S' Is there any plans to support sign?

Pavel-Husakouski avatar Feb 16 '21 12:02 Pavel-Husakouski