Lib should error instead of stripping invalid characters

[ericvera]

I found an issue where '123.sd' is turned into '123.00'. I was expecting this to error out as it is not a valid currency value. Looking at the code I found the following test. Any insights of what is the logic of such functionality?

If the functionality has come valid case, would it be possible to have a flag that will make this case error out?

test('should strip invalid characters', t => {
  var str = currency('a1b2c3');
  t.is(parseFloat(str), 123, 'currency("a1b2c3") is 123');
});

[scurker]

One of the key features of currency.js is in how it accepts various inputs. a1b2c3 is probably not the best example of a test, but it's more so that the library can accept a wide range of currency formats. Internally, it's pretty agnostic on how input is parsed so if your currency string is € 1 234 567,89 or $ 1,234,567.89 it can just take the number respective of the decimal format and output something currency like regardless of what your delimiters or symbols are. Changing it to output an error would be significant breaking change at this stage.

There is a an existing errorOnInvalid flag that exists but it currently only errors on null or undefined values. It's possible we could co-opt that, but it would be difficult to determine what's a valid string without adding a lot of additional weight to the library.

[paintedbicycle]

I really appreciate the flexible nature of the input string and how the lib is able to deal with it, so I wouldn't want to lose that.

Perhaps one step toward this would be to error on letters? Stripping out spaces, commas, periods and currency symbols seems normal, but having the letter d in there seems weird. Perhaps though, it's to allow a currency code in the input string? I didn't see a reference to doing this anywhere in the docs, so perhaps it'd be possible to add this small check without adding the additional weight.

[scurker]

Unfortunately, it's not universal that a symbol is used for currency symbols. Several countries use kr as their symbol for krona or krone, while you might see USD $1 or CAN $1 to distinguish between multiple countries that use the same symbol. It's also not uncommon to drop the symbol all together in place of a country code.

However, I'd love to better understand the use cases where this would be valuable.

[ericvera]

In my case since, at least at the moment, I only care about USD I just added a separate validation step before introducing currency.js into the equation using a regular expression. What I was trying to accomplish was to provide directly the user input to currency.js and have that be the "this is a valid currency string" check. While I am familiar with many different currencies and have seen the cases you are describing I wouldn't say I know enough to say more restrictive is better. I am a huge fan of the library so far and its simplicity. Having said that a few ideas come to mind: - any characters before the first number should be stripped, then within the number only separators and spaces are stripped. I don't know if there is a currency that does something like 1 dollar. 30 cents or something like that where letters or currency symbols would be useful in between numbers. - having a strict mode or something that does not allow any of those. Perhaps just the numbers, maybe separators and spaces, but no currency with the assumption that the consumer of the library is responsible for all the country specific details.

…

On Wed, Mar 13, 2019, 5:59 PM Jason ***@***.***> wrote: Unfortunately, it's not universal that a symbol is used for currency symbols. Several countries use kr as their symbol for *krona* or *krone*, while you might see USD $1 or CAN $1 to distinguish between multiple countries that use the same symbol. It's also not uncommon to drop the symbol all together in place of a country code. However, I'd love to better understand the use cases where this would be valuable. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#184 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGinB7egKHgWwyr7YlDgmvAF-YaM2jW1ks5vWZ7pgaJpZM4bf6rQ> .

[scurker]

@ericvera I think a having strictInput or strict mode actually sounds reasonable. I need to think about what that means in relation to errorOnInvalid but that's something that could potentially be released with a 2.0 version of the library.

[osddeitf]

it's v2.0.4 as of now, shouldn't this be implemented?