-
Notifications
You must be signed in to change notification settings - Fork 68
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
Load sprintf.js via npm #46
Conversation
This requires the npm module instead of inlining it and updates sprintf.js from 0.7-beta1 to version 1.0.3. A few changes to the tests were required to accomodate for the version bump.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. The two reasons I inline sprintf in the past were because of the undefined
behavior here, and because it wasn't on npm at the time I wrote this :).
Would love to not have to change this undefined behavior, it's never what people want.
What do you think about throwing an error instead of just turning I'd be happy to add support for this to the sprintf library if that sounds good to you. |
Is it really an error to have empty values? I think runtime errors in
|
@SlexAxton Just stumbled upon this again. What do you think about using the latest version of |
Happy to take a PR for that.
…On Fri, Aug 10, 2018 at 1:36 AM Pascal Birchler ***@***.***> wrote:
@SlexAxton <https://github.com/SlexAxton> Just stumbled upon this again.
What do you think about using the latest version of sprintf from npm (see
also #56 <#56>) and then wrap
a little helper function around them to handle undefined? Happy to submit
a PR.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#46 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAF5Km0csLK_t3uHpEUmNk1XGXlYF7qsks5uPSntgaJpZM4KE7s0>
.
|
This requires the sprintf.js npm module instead of inlining it and updates the library from 0.7-beta1 to version 1.0.3. A few changes to the tests were required to accommodate for the change.
See #41.