Ability to round up/down in multiply and divide operations

[gidonkatten]

The default behaviour for the multiply and divide operations was to ROUND_HALF. Added the possibility to specify whether to ROUND_UP or ROUND_DOWN. Rounding down is especially important as that is the typical behaviour in smart contract opcodes (EVM and non-EVM alike).

Also fixed a bug in the divideAndRound function (which is called by many operations) when the divisor is negative. E.g. divideAndRound(-15n, 2n)) and divideAndRound(15n, -2n) yielded different results.

Updated docs with latest changes.

[bpierre]

Thanks for adding this, it’s looking great! And thanks for fixing the rounding bug 🙏

What would you think of having optionsOrDecimals as a third parameter, following what we are doing with other functions using optionsOrDigits?

Also, would it be worth adding the ability to pass a Rounding to dnum.round()?

[gidonkatten]

Thanks @bpierre for looking at the PR, replying to your comments

What would you think of having optionsOrDecimals as a third parameter, following what we are doing with other functions using optionsOrDigits?

Sorry I don't understand what this means, can you elaborate?

Also, would it be worth adding the ability to pass a Rounding to dnum.round()?

Done :)

[gidonkatten]

Actually I now realise the library already has floor and ceil functions so not sure how much value adding ROUND_UP and ROUND_DOWN to dnum.round() brings. Feel free to suggest to remove it if you prefer

[bpierre]

What would you think of having optionsOrDecimals as a third parameter, following what we are doing with other functions using optionsOrDigits?

Sorry I don't understand what this means, can you elaborate?

I mean doing this:

export function divide(
  num1: Numberish,
  num2: Numberish,
  optionsOrDecimals?: Decimals | {
    decimals?: Decimals,
    rounding?: Rounding,
  } = {},
): Dnum {
  const options = typeof optionsOrDecimals === "number"
    ? { decimals: optionsOrDecimals }
    : optionsOrDecimals

  options.rounding ??= "ROUND_HALF"
  // …
}

To follow the API of e.g. format():

export function format(
  dnum: Dnum,
  // see toParts() in src/dnum.ts
  optionsOrDigits: Parameters<typeof toParts>[1] & {
    compact?: boolean;
    locale?: ConstructorParameters<typeof Intl.NumberFormat>[0];
    signDisplay?: SignDisplay;
  } = {},
): string {
  const options = typeof optionsOrDigits === "number"
    ? { digits: optionsOrDigits }
    : optionsOrDigits;
  // …

The general idea being to pick one option from the options object and allow it to be passed directly as a shortcut.

Also, would it be worth adding the ability to pass a Rounding to dnum.round()?

Done :)
Actually I now realise the library already has floor and ceil functions so not sure how much value adding ROUND_UP and ROUND_DOWN to dnum.round() brings. Feel free to suggest to remove it if you prefer

It does yes, but I thought it could be nice to allow using Rounding here too, and have ceil() and floor() as aliases? Or do you think it will just add confusion?

[gidonkatten]

What would you think of having optionsOrDecimals as a third parameter, following what we are doing with other functions using optionsOrDigits?

Okay I understand now - I don't really have a preference. I followed what you had done for setDecimals where the decimals and options were seperated.

It does yes, but I thought it could be nice to allow using Rounding here too, and have ceil() and floor() as aliases? Or do you think it will just add confusion?

I agree it's probably a nice thing to have and I don't see it being confusing. Now that you mentioned it, I simplified ceil and floor functions to call round.

[bpierre]

Okay I understand now - I don't really have a preference. I followed what you had done for setDecimals where the decimals and options were seperated.

Thinking more about it, I think it would be weird to require consumers to set decimals (or explicitly pass undefined) in order to set options.rounding. Let’s go with optionsOrDecimals in multiply(), divide() and round(), if you don’t mind making the change 🙏

It does yes, but I thought it could be nice to allow using Rounding here too, and have ceil() and floor() as aliases? Or do you think it will just add confusion?

I agree it's probably a nice thing to have and I don't see it being confusing. Now that you mentioned it, I simplified ceil and floor functions to call round.

Nice! We might even end up with a smaller size than before 😄

[gidonkatten]

@bpierre I updated the three operations with optionsOrDecimals like you suggested. The README needs to be updated with the new parameter, how would you advise to do this?

[bpierre]

@gidonkatten looking good!

For the docs: maybe we could do it the same way as format(), where options.digits says this:

“Setting options to a number acts as an alias for this option (see example below).”.

[bpierre]

Thanks a lot @gidonkatten 🙏