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

doc: confusing language in util.format() docs #12362

Closed
q42jaap opened this issue Apr 12, 2017 · 19 comments
Closed

doc: confusing language in util.format() docs #12362

q42jaap opened this issue Apr 12, 2017 · 19 comments
Labels
doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. util Issues and PRs related to the built-in util module.

Comments

@q42jaap
Copy link

q42jaap commented Apr 12, 2017

214d020

The intention to speed up util.format breaks the following case:

util.format('%%')

prior to this commit, this returned '%', now it returns '%%'.

Imho, this is a bug and should be reverted.

@aqrln aqrln added the util Issues and PRs related to the built-in util module. label Apr 12, 2017
@q42jaap
Copy link
Author

q42jaap commented Apr 12, 2017

This works correctly in node 0.12
broken in node 4 and up

@evanlucas
Copy link
Contributor

to me, if util.format('%%') is supposed to return %, then util.format('%s') would return s. I don't really think the previous return value is intuitive. At this point, I'm not sure changing it back would be worth it. Is there a scenario where this is causing a problem for you?

@mscdex
Copy link
Contributor

mscdex commented Apr 12, 2017

FWIW it's only an issue when you don't pass any additional arguments to format().

@addaleax
Copy link
Member

addaleax commented Apr 12, 2017

The current behaviour also matches the one that browsers have in console.log, fwiw.

@evanlucas The idea is been that if strings like %s are used as placeholders, then %% is the way to escape a percent sign. It’s been that way since printf() was invented, and that’s what most languages do, so I would suggest to not deviate in Node. (edit: maybe I misunderstood you – I assumed you were talking about the formatting function of util.format for multiple arguments)

@aqrln
Copy link
Contributor

aqrln commented Apr 12, 2017

If the first argument is not a format string then util.format() returns a string that is the concatenation of all arguments separated by spaces. Each argument is converted to a string using util.inspect().

(https://nodejs.org/dist/latest-v7.x/docs/api/util.html#util_util_format_format_args)

I'd rather we clarify it here that if a string is the only argument, it cannot be a format string by definition, so it is returned as is.

@evanlucas
Copy link
Contributor

@addaleax I probably should have clarified. I was thinking only in the case of a single argument.

@addaleax
Copy link
Member

Yeah, I realized too late that I probably misunderstood you. 😄

@aqrln
Copy link
Contributor

aqrln commented Apr 12, 2017

So how about making it a good first contribution documentation issue (#12362 (comment))?

@q42jaap
Copy link
Author

q42jaap commented Apr 12, 2017

I still think, that util.format should behave more like printf in C than console.log in javascript.

@q42jaap
Copy link
Author

q42jaap commented Apr 12, 2017

The case is winston logging a sql query with wildcards WHERE column LIKE '%string_with_s%' which I had to escape to WHERE column LIKE '%%string_with_s%%'.
winston calls util.format.apply(null, args), which in my case uses just 1 parameter.

Given that there wasn't a testcase allowing the performance improvement to break code unnoticed, I'd still opt for printf behavior over console.log.

@q42jaap
Copy link
Author

q42jaap commented Apr 12, 2017

#include <stdio.h>

int main() {
  printf("%%\n");
}

will print % not %%.

@bnoordhuis
Copy link
Member

What printf() does is irrelevant though, util.format() is used to implement console.log() and that prints '%%' in browsers and node. Moving to close.

@q42jaap
Copy link
Author

q42jaap commented Apr 12, 2017

@bnoordhuis the docs mention printf specifically... So it is definitely not irrelevant. Furthermore, the bug was unintentionally introduced in given commit. There should have been a testcase.
Other than the mentioning of printf, the docs lay out a clear path for how tokens are processed. %% is a token that does not consume an argument and just prints %.

This is just wrong!

@bnoordhuis
Copy link
Member

I'm willing to concede the docs are wrong (didn't check) but the behavior itself brings node closer to browsers. I see no reason to roll that back. "It's not like printf" is not a compelling argument to me.

@Fishrock123 Fishrock123 added the doc Issues and PRs related to the documentations. label Apr 12, 2017
@Fishrock123
Copy link
Contributor

I guess this is a docs issue?

@aqrln aqrln added the good first issue Issues that are suitable for first-time contributors. label Apr 12, 2017
@aqrln
Copy link
Contributor

aqrln commented Apr 12, 2017

Yeah, definitely a docs issue IMO. I think the current behavior is correct given that util.format() is just a backend for console.log() in the first place and it has never been a printf() counterpart (that falls outside the scope of Node core and it's quite feasible to do npm install printf or other userland package, I believe there should be a plenty of them). Not to mention that it's better to be incompatible with EOL v0.12 than with modern Node.js versions that have been released for the past two years.

@aqrln aqrln changed the title util.format was broken 2 years ago in an attempt to gain performance doc: confusing language in util.format() docs Apr 12, 2017
@tarunbatra
Copy link
Contributor

Hey, I'd like to help with this! 🙋
Just need to know if we'd want to remove the reference to printf, apart from documenting the single argument scenario?

@aqrln
Copy link
Contributor

aqrln commented Apr 12, 2017

@tarunbatra great, go for it! You can ping me here, in #node-dev on Freenode or via direct messages on Twitter (username is the same) if you need any help.

I'd say printf() references are fine as long as it is clear that when the first argument is considered to be a format string, it has printf-like-ish (though far from what printf can do) format, nothing more. If it occurs to be hard to formulate it concisely, I'm fine with removing references to printf() completely and saying just "format string" instead of "printf-like format string". I think I would even prefer it :)

Open a PR though, and reviewers will help find the best wording :)

@tarunbatra
Copy link
Contributor

Thanx @aqrln! I created #12374.

I skipped the changes to printf() thinking that it's reference helps devs from other backgrounds to get instantly familiar with the method. Will change it if you feel so.

evanlucas pushed a commit that referenced this issue Apr 25, 2017
Set the expected outcome of `util.format('%%')` to be `%%`
instead of `%`.

PR-URL: #12374
Fixes: #12362
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
evanlucas pushed a commit that referenced this issue May 1, 2017
Set the expected outcome of `util.format('%%')` to be `%%`
instead of `%`.

PR-URL: #12374
Fixes: #12362
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
evanlucas pushed a commit that referenced this issue May 2, 2017
Set the expected outcome of `util.format('%%')` to be `%%`
instead of `%`.

PR-URL: #12374
Fixes: #12362
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this issue May 15, 2017
Set the expected outcome of `util.format('%%')` to be `%%`
instead of `%`.

PR-URL: #12374
Fixes: #12362
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this issue May 18, 2017
Set the expected outcome of `util.format('%%')` to be `%%`
instead of `%`.

PR-URL: #12374
Fixes: #12362
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
andrew749 pushed a commit to michielbaird/node that referenced this issue Jul 19, 2017
Set the expected outcome of `util.format('%%')` to be `%%`
instead of `%`.

PR-URL: nodejs/node#12374
Fixes: nodejs/node#12362
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants