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.

Sign up for GitHub

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

Jump to bottom

v6.x backport of 'latin1' encoding #8022

Merged
merged 4 commits into from
Aug 10, 2016
Merged

v6.x backport of 'latin1' encoding #8022

merged 4 commits into from
Aug 10, 2016

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Aug 8, 2016 •
edited
Loading

Backport of #7111, half of #7262 (the other half already landed) and #7284.

/cc @nodejs/buffer

CI: https://ci.nodejs.org/job/node-test-commit/4461/

trevnorris and others added 4 commits August 8, 2016 21:37
@trevnorris @addaleax
buffer: introduce latin1 encoding term
28071a1 
When node began using the OneByte API (f150d56) it also switched to
officially supporting ISO-8859-1. Though at the time no new encoding
string was introduced.

Introduce the new encoding string 'latin1' to be more explicit. The
previous 'binary' and documented as an alias to 'latin1'.  While many
tests have switched to use 'latin1', there are still plenty that do both
'binary' and 'latin1' checks side-by-side to ensure there is no
regression.

PR-URL: nodejs#7111
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@trevnorris @addaleax
src: remove final trace of raw encoding
a059aea 
A message stuck around in the native API warning users to not use 'raw'
encoding. Followed by an abort(). This is no longer necessary since all
other signs of 'raw' encoding have been removed.

PR-URL: nodejs#7111
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@bnoordhuis @addaleax
src: fix erroneous fallthrough in ParseEncoding()
7ba0f86 
A missing 'break' statement unintentionally allowed "linary"
and "luffer" as alternatives for "binary" and "buffer".

Regression introduced in commit 54cc721 ("buffer: introduce latin1
encoding term".)

PR-URL: nodejs#7262
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@bnoordhuis @addaleax
src: alias BINARY to LATIN1
da9bd2f 
Make BINARY an alias for LATIN1 rather than a distinct enum value.

PR-URL: nodejs#7284
Refs: nodejs#7262
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@addaleax addaleax added buffer Issues and PRs related to the buffer subsystem. semver-minor PRs that contain new features and should be released in the next minor version. v6.x labels Aug 8, 2016
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. build Issues and PRs related to build files or the CI. lib / src Issues and PRs related to general changes in the lib or src directory. labels Aug 8, 2016
@addaleax addaleax removed the build Issues and PRs related to build files or the CI. label Aug 8, 2016
@addaleax addaleax mentioned this pull request Aug 8, 2016
Closed
@cjihrig
Copy link
Contributor

cjihrig commented Aug 9, 2016

Looks like there were some Windows build issues.

@addaleax are you particularly anxious to get this into v6.4.0?

@addaleax
Copy link
Member Author

addaleax commented Aug 9, 2016

@cjihrig It wouldn’t be the end of the world if it doesn’t make it. 😉 Though, I think I can speak for me and at least @trevnorris in that we both would really like to see it landed in v6 at some point, because we see quite the value in moving away from binary as a very confusingly named encoding identifier.

@addaleax
Copy link
Member Author

addaleax commented Aug 9, 2016

I think I have seen these Windows errors before in an unrelated context, so running CI again: https://ci.nodejs.org/job/node-test-commit/4477/

trevnorris
trevnorris reviewed Aug 9, 2016
View reviewed changes
doc/api/buffer.md

* `'hex'` - Encode each byte as two hexadecimal characters.

_Note_: Today's browsers follow the [WHATWG
spec](https://encoding.spec.whatwg.org/) that aliases both `latin1` and
`iso-8859-1` to `win-1252`. Meaning, while doing something like `http.get()`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quick note, cc1318b contains documentation changes for these entries. not sure if it's worth while to also back port those.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if this PR lands the commits from #7784 should land (mostly) cleanly and will be included automatically then? cc1318b definitely does, so I’d expect it to be picked up. I can add that one here if you think it makes reviewing easier?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not worried about adding them in this PR. i don't even feel particularly strong about landing the doc changes. just thought it be useful to bring it up.

@trevnorris
Copy link
Contributor

One comment/question. LGTM.

@addaleax addaleax merged commit da9bd2f into nodejs:v6.x Aug 10, 2016
@addaleax addaleax deleted the latin1-v6.x branch August 10, 2016 13:58
@Fishrock123
Copy link
Contributor

Fishrock123 commented Aug 16, 2016 •
edited
Loading

These commits should have been PR-URL: https://github.com/nodejs/node/pull/8022 to prevent tooling from picking up the wrong labels.

The old one probably should have been Refs:'d or something.

@addaleax
Copy link
Member Author

@Fishrock123 Thanks for the comments, I wasn’t aware of that :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@addaleax @cjihrig @trevnorris @Fishrock123 @nodejs-github-bot @bnoordhuis