doc: fix misleading ASCII comments #11657
Conversation
nodejs-github-bot
added
buffer
doc/api/buffer.md
Outdated
| const buf6 = Buffer.from('tést', 'utf8'); | ||
| const buf5 = Buffer.from('tést'); | ||
|
|
||
| // Creates a Buffer containing ASCII bytes [0x74, 0xe9, 0x73, 0x74]. |
There was a problem hiding this comment.
These are not ASCII bytes, though?
There was a problem hiding this comment.
Not sure I understand, did I make an error in the result?
There was a problem hiding this comment.
ASCII encoding uses only the bytes up to 127, and it can’t encode characters such as é at all. In some places of the API – like Buffer.from(…, 'ascii') – Node just treats ascii like latin-1 for performance reasons, where é maps to 0xe9. That doesn’t always work, though; for example:
> Buffer.from('tést', 'ascii').toString('ascii')
'tist'If you want to use this example, I’d suggest going with something like Latin-1 bytes, and use Buffer.from(…, 'latin-1').
There was a problem hiding this comment.
Ah okay, wasn't aware of that. I'll change it to latin-1.
| @@ -38,11 +38,11 @@ const buf3 = Buffer.allocUnsafe(10); | |||
| // Creates a Buffer containing [0x1, 0x2, 0x3]. | |||
| const buf4 = Buffer.from([1, 2, 3]); | |||
|
|
|||
| // Creates a Buffer containing ASCII bytes [0x74, 0x65, 0x73, 0x74]. | |||
| const buf5 = Buffer.from('test'); | |||
There was a problem hiding this comment.
Why remove this example? I think I can see where this is coming from, but it might be better to just replace ASCII with ASCII/UTF-8 in the comment?
There was a problem hiding this comment.
My reasoning was that it's more consistent with the other examples in the doc, and also shows the difference between the two encodings that will affect the output ('test' is the same in ascii and utf8 but not 'tést'). That may or may not be important
Just changing the text to ASCII/UTF-8 is still an improvement but I think this example is clearer.
|
Landed in bbc118f, thanks for the PR! 🎉 |
PR-URL: #11657 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
PR-URL: #11657 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
PR-URL: #11657 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
|
backported to v6.x... lmk if I was mistaken to do so |
PR-URL: #11657 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
PR-URL: nodejs/node#11657 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
Fixed a couple of misleading references to ASCII for default encoding examples (should be UTF-8).
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
doc