search

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.96k stars 530 forks source link

updated formatColumn to only accept an array as the first argument #160

Closed jjsquillante closed 7 years ago

jjsquillante commented 7 years ago

@josscrowcroft Great job on accounting.js. It's been a pleasure reading through the code. Please let me know if I can send some money to buy you a cup of coffee or beer!

As for the pull request: When a string value is passed as the first argument, every character is returned as a value in the array which can cause unwanted results.

accounting.formatColumn('1,234.50')
[$1.00,$0.00,$2.00,$3.00,$4.00,$0.00,$5.00,$0.00]

According to your comments in the code and the qunit tests, the first argument should only be an array of numbers.

I propose adding a check to detect if the first argument is not an array and, if true, return an empty array. This matches scenarios like if a number, function, or object is passed as the first argument. (all return an empty array)

wjcrowcroft commented 7 years ago

@jjsquillante Thanks and good catch! There's a good number of outstanding PRs and issues for this library, which I'm hoping to get incorporated over the coming month or two. This one seems like a good sensible place to start getting things up-to-date..

jjsquillante commented 7 years ago

@josscrowcroft awesome news!!

Like I said, I've really enjoyed combing through your code for accounting.js so if there's anything else I notice or something I can add (noticed some comments about improving errors from the current state of failing silently), I'll be sure to submit another PR to help out.

hope you enjoy your day/night.