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

Suggesting an inconsistency inside defaults function #178

Open elenaparaschiv opened 7 years ago

elenaparaschiv commented 7 years ago

There is an inaccuracy regarding the comment in line 87, regarding allowing for empty/ zero values

// Replace values with defaults only if undefined (allow empty/ zero values) 

Problem: Calling the defaults function, if object has a value of null, its value will be overwritten by the defs value. Below is an example:

defaults({color: null}, {color:'grey', wheels:2})  
 // returns {color: "grey", wheels: 2}

This is inconsistent with the comment, which should allow for empty/ zero values. To fix it, null can be swapped for undefined.

Solution: With this fix, the expected output is returned.

{color: null, wheels: 2},

If the authors consider to not replace this code due to reasons such that it might brake the code for existing users of accounting.js, I suggest to at least change the comment to be more accurate, for example:

// Replace values with defaults only if undefined.

Thanks to @gordonmzhu for the inspiration behind this pull request.