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
Update sprintf and vsprintf to make use of the sprintf-js package. #479
Conversation
The owner of the current implementation of sprintf has continued development and improved the implementation. It makes sense to use of his package on npm instead of using an out-of-date copy.
Yes. We should add a warning and a link when this function is being used in next minor release and then completely remove it in next major. |
@jtangelder could you add a warning to your pr? |
Sure, do you have a function for this? |
I would use https://www.npmjs.com/package/util-deprecate. @epeli also good with this? |
yep |
OK, will add it tomorrow! |
Cool, thank you! Jorik Tangelder notifications@github.com schrieb am Mo., 18. Jan. 2016 um
|
I've updated the PR |
@jtangelder thanks. We should probably add a warning (and link) to the README as well. Sorry that I didn't mention that in my comment before 🙇 |
63a5120
to
40d16b0
Compare
No problem, added it to the previous commit 💃 |
|
||
[o]: http://www.diveintojavascript.com/projects/javascript-sprintf | ||
**This function will be removed the next major release, use the [sprintf-js](https://npmjs.org/package/sprintf-js) package instead.** |
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.
-This function will be removed the next major release
+This function will be removed in the next major release
Nice. There are two minor typos left. Could you fix them an squash your commits. |
40d16b0
to
f333164
Compare
Fixed them! |
|
||
module.exports = sprintf; | ||
module.exports = deprecate(require('sprintf-js').sprintf, | ||
'sprintf() will be removed the next major release, use the sprintf-js package instead.'); |
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.
still missing in
will be removed in the
arg didn't see the missing |
f333164
to
026a9f0
Compare
Got it ^ |
Awesome thanks. |
Update sprintf and vsprintf to make use of the sprintf-js package.
The owner (@alexei) of the current implementation of sprintf has continued development and improved the implementation. It makes sense to use of his package on npm instead of using an out-of-date copy.