Skip to content
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

Merged
merged 2 commits into from Jan 19, 2016

Conversation

jtangelder
Copy link
Contributor

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.

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.
@stoeffel
Copy link
Collaborator

I would vote for removing sprintf completely and just add a link to @alexei's module. Less code to maintain and we don't have to copy the changes from the origin to our module. @epeli do you agree?

@esamattis
Copy link
Owner

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.

@stoeffel
Copy link
Collaborator

@jtangelder could you add a warning to your pr?

@jtangelder
Copy link
Contributor Author

Sure, do you have a function for this?

@stoeffel
Copy link
Collaborator

I would use https://www.npmjs.com/package/util-deprecate. @epeli also good with this?

@esamattis
Copy link
Owner

yep

@jtangelder
Copy link
Contributor Author

OK, will add it tomorrow!

@stoeffel
Copy link
Collaborator

Cool, thank you!

Jorik Tangelder notifications@github.com schrieb am Mo., 18. Jan. 2016 um
21:06 Uhr:

OK, will add it tomorrow!


Reply to this email directly or view it on GitHub
#479 (comment)
.

@jtangelder
Copy link
Contributor Author

I've updated the PR

@stoeffel
Copy link
Collaborator

@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 🙇

@jtangelder
Copy link
Contributor Author

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.**
Copy link
Collaborator

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

@stoeffel
Copy link
Collaborator

No problem, added it to the previous commit 💃

Nice.

There are two minor typos left. Could you fix them an squash your commits.
Then it's good to 🚢. Thanks

@jtangelder
Copy link
Contributor Author

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.');
Copy link
Collaborator

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

@stoeffel
Copy link
Collaborator

arg didn't see the missing in until now. Sorry, could you fix that as well?

@jtangelder
Copy link
Contributor Author

Got it ^

@stoeffel
Copy link
Collaborator

Awesome thanks.

stoeffel added a commit that referenced this pull request Jan 19, 2016
Update sprintf and vsprintf to make use of the sprintf-js package.
@stoeffel stoeffel merged commit 93dffd5 into esamattis:master Jan 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants