doc: fix misleading ASCII comments - #11657
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
These are not ASCII bytes, though?
Sorry, something went wrong.
There was a problem hiding this comment.
Not sure I understand, did I make an error in the result?
Sorry, something went wrong.
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').
Sorry, something went wrong.
There was a problem hiding this comment.
Ah okay, wasn't aware of that. I'll change it to latin-1.
Sorry, something went wrong.
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?
Sorry, something went wrong.
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.
Sorry, something went wrong.
addaleax
left a comment
There was a problem hiding this comment.
Thanks, looks good to me!
Sorry, something went wrong.
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 |
Sorry, something went wrong.
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
Affected core subsystem(s)
doc