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

Always consume the whole format string #757

Closed
wants to merge 1 commit into from
Closed

Always consume the whole format string #757

wants to merge 1 commit into from

Conversation

vhf
Copy link

@vhf vhf commented Mar 18, 2019

As highlighted here by @rauschma , there seems to be a bug in formatWithOptions:

node -r esm -e "const {format} = require('util'); console.log(format('%s%%', 99))"
99%%

formatWithOptions stops consuming the format string and simply copies the rest of its input instead of processing it as soon as all formats have been replaced by their arg. In the above case, there's one format and one arg so the rest (%%) gets copied. Instead, %% should be processed as % as stated in node.js docs.

(Since it's a special case (the only 'format' that takes 0 arg), this PR could be made a bit more efficient by having special detection for this case and break out early as it was before but after making sure this case is handled. Not sure it's worth it though, it would add quite a few lines of code thus making it more complex for a seemingly small benefit.)

@rauschma
Copy link

Possibly: write a unit test to prevent a regression.

@vhf
Copy link
Author

vhf commented Mar 18, 2019

I thought about it and looked for unit tests for this file/grepped the function name but couldn't find any, I'll add one if there are already some.

@jdalton jdalton closed this in 5cd561d Mar 19, 2019
@jdalton jdalton added the bug label Mar 19, 2019
@jdalton
Copy link
Member

jdalton commented Mar 20, 2019

Patch 5cd561d; Test 1d93fb3.

Update:

esm v3.2.19 is released 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants