openexchangerates / accounting.js

A lightweight JavaScript library for number, money and currency formatting - fully localisable, zero dependencies.
http://openexchangerates.github.io/accounting.js
MIT License
4.95k stars 528 forks source link

Clarify unformat() usage (Issue #175), and fix bug related to using non-standard separators without changing default settings object (Issue #129). #186

Closed thomasjlee closed 6 years ago

thomasjlee commented 6 years ago

Edited (26 November 2017): There are additional issues related to passing number as strings to formatMoney and formatNumber, so I have closed the pull request for now.

Important: Changes to accounting.js are not reflected in the minified version.

Issues Addressed

#175 - To call unformat() on a number that uses a decimal separator other than "." (eg. a comma), user must pass in the decimal separator as the second argument to unformat().

Example: accounting.unformat("100,00", ",") // 100

This is not a bug. However, the current docs do not provide sufficient examples for the unformat method. I have updated the description of unformat() in the "Library Methods" section of the documentation with example code, as well as added QUnit tests to better demonstrate this usage.

#129 - Some users who handle currencies which use "." as thousand separators and "," as decimal separators may prefer to pass numbers as strings into formatMoney() and formatNumber() because JavaScript will not appropriately recognize a float such as 1.234.567,89.

If the default settings object is altered prior to using formatMoney() or formatNumber(), this works just fine:

accounting.settings.currency = {
  symbol: "R$",
  decimal: ",",
  thousand: ".",
  format: "%s %v"
};

accounting.settings.number = {
  precision: 2,
  decimal: ",",
  thousand: "."
};

accounting.formatMoney("123,45");    // "R$ 123,45"
accounting.formatMoney("1.234,56");  // "R$ 1.234,56"

accounting.formatNumber("123,45");   // "123,45"
accounting.formatNumber("1.234,56"); // "1.234,56"

However, should a user prefer not to alter the settings object (and rather pass an options object directly), this issue arises:

accounting.formatMoney("123,45", { 
  symbol: "R$",
  thousand: ".",
  decimal: ",",
  format: "%s %v"
}); // "R$ 12.345,00" (expected: "R$ 123,45")

accounting.formatMoney("1.234,56", {
  symbol: "R$",
  thousand: ".",
  decimal: ",",
  format: "%s %v" 
}); // "R$ 1,23" (expected: "R$ 1.234,56")

The same issue arises in formatNumber():

accounting.formatNumber("123,45", {
  precision: 2,
  thousand: ".",
  decimal: ","
}); // "12.345,00" (expected: "123,45")

accounting.formatNumber("1.234,56", {
  precision: 2,
  thousand: ".",
  decimal: ","
}); // "1,23" (expected: "1.234,56")

These issues occur even if options are passed individually, rather than as an object.

When exploring this issue, I noticed a rather simple change would fix it:

Rather than adding a specific check for the usage of the Brazilian currency symbol ("R$") as proposed by PR #183, this subtle change will handle all cases where number is passed as string and non-default separators are used.

With the changes:

accounting.formatMoney("123,45", { 
  symbol: "R$",
  thousand: ".",
  decimal: ",",
  format: "%s %v"
}); // "R$ 123,45"

accounting.formatMoney("1.234,56", {
  symbol: "R$",
  thousand: ".",
  decimal: ",",
  format: "%s %v" 
}); // "R$ 1.234,56"

accounting.formatNumber("123,45", {
  precision: 2,
  thousand: ".",
  decimal: ","
}); // "123,45"

accounting.formatNumber("1.234,56", {
  precision: 2,
  thousand: ".",
  decimal: ","
}); // "1.234,56"

I have also added additional QUnit and Jasmine tests to demonstrate this minor change works.

Final Considerations