New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace formatNumber with toLocaleString #682
Comments
The current function doesn't seem to have a handler for non-numbers. Now that we're refactoring it, we might want to fix that. How should the function handle such (non-number) inputs? |
What do you recommend? I always like to hear your idea first, unless you're truly stumped. |
Based on it's existing usage, I'd suggest we should ignore |
Okay, that's an option. I'm a little concerned that that might allow some invalid usage to slip through. Wouldn't it be better to return |
Initially, I thought of the same options. Throwing an error or returning
null would require explicit handling by the caller.
Unless it deviates from the business requirements, a more appropriate way
would be to ensure that the input to this function is only number(can be
done using TS) as it's responsibility is to format the number.
If we need the function to work for strings, we can add some logic for that
too using TS.
…On Mon, May 25, 2020, 22:40 Raine Revere ***@***.***> wrote:
Okay, that's an option. I'm a little concerned that that might allow some
invalid usage to slip through. Wouldn't it be better to return null or
throw an error?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#682 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB3H6BERR4KAQ665Z7H4YT3RTKRB3ANCNFSM4NH6ARJQ>
.
|
Makes sense that it should only accept |
#656 (review)
The text was updated successfully, but these errors were encountered: