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

[buffer]: introduce the 'latin1' encoding string #7111

Merged
merged 2 commits into from
Jun 7, 2016

Conversation

trevnorris
Copy link
Contributor

@trevnorris trevnorris commented Jun 2, 2016

  • tests and code linting passes
  • a test and/or benchmark is included
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

buffer, string_bytes

Description of change

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' is still around and only deprecated in documentation.
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.

There's another minor cleanup commit included.

R=@bnoordhuis
R=@addaleax
R=@jasnell

CI: https://ci.nodejs.org/job/node-test-pull-request/2902/

@trevnorris trevnorris added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Jun 2, 2016
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jun 2, 2016
@Fishrock123
Copy link
Contributor

I think this is a good idea and a slow deprecation cost we should swallow so that things are make sense for the future.

@bnoordhuis
Copy link
Member

LGTM

@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 2, 2016
@jasnell
Copy link
Member

jasnell commented Jun 2, 2016

semver-major because of the docs-only deprecation of binary

@addaleax
Copy link
Member

addaleax commented Jun 2, 2016

@trevnorris Might be a good idea to separate out the deprecation to make this a semver-minor?

LGTM, and a big thanks for doing this.

@jasnell
Copy link
Member

jasnell commented Jun 2, 2016

LGTM with green CI and CITGM. That said, not really sure there's a ton of benefit here as we'll likely never be able to get rid of the binary value.

@addaleax
Copy link
Member

addaleax commented Jun 2, 2016

I don’t think anybody’s expecting to remove binary, but I’m pretty sure there’s a benefit for learners in this. People referring to “binary strings” just adds a lot of confusion to the topic of character encodings, and it’s already a confusing topic all by itself.

@trevnorris
Copy link
Contributor Author

trevnorris commented Jun 2, 2016

@jasnell didn't realize doc only deprecation would be semver-major. if that's the case can I just remove (deprecated) for a semver-minor patch and re-add it for next major.

edit: to clarify, reason for deprecation is as @addaleax pointed out. to simply point out that 'binary' encoding isn't actually a thing, and for clarity should use 'latin1'.

@jasnell
Copy link
Member

jasnell commented Jun 2, 2016

All other doc only deprecations we've done have been treated as semver-majors as well.
Removing the deprecation would certainly bump this down to a minor.

@trevnorris
Copy link
Contributor Author

@jasnell There you go. Changed documentation to state that 'binary' is an alias for 'latin1'.

@jasnell jasnell added semver-minor PRs that contain new features and should be released in the next minor version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Jun 6, 2016
@trevnorris
Copy link
Contributor Author

trevnorris commented Jun 7, 2016

@jasnell This mean I'm good to land?

CI after rebase: https://ci.nodejs.org/job/node-test-commit/3685/

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.
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.
@jasnell
Copy link
Member

jasnell commented Jun 7, 2016

Lgtm

@trevnorris trevnorris merged commit a7bd2a6 into nodejs:master Jun 7, 2016
trevnorris added a commit to trevnorris/node that referenced this pull request Jun 7, 2016
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
trevnorris added a commit to trevnorris/node that referenced this pull request Jun 7, 2016
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@trevnorris
Copy link
Contributor Author

Thanks much! Landed in 54cc721 and e0b8dd5.

@bnoordhuis bnoordhuis added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-minor PRs that contain new features and should be released in the next minor version. labels Jun 17, 2016
@bnoordhuis
Copy link
Member

I'm changing this to semver-major again because the change to the encoding enum is an ABI change; the values of the enum change.

It could land in concert with #7284 as a semver-minor change but honestly, I'd just wait until v7.

@trevnorris
Copy link
Contributor Author

I know it's been almost two years since this should have been introduced, but that means one more LTS of a confusing encoding name =/

If it can't be done then I'll deal with it, but think it'd be useful to land in v6.

@addaleax
Copy link
Member

I’d be +1 for landing these together in a semver-minor, too.

@addaleax addaleax mentioned this pull request Aug 8, 2016
addaleax pushed a commit to addaleax/node that referenced this pull request Aug 8, 2016
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit to addaleax/node that referenced this pull request Aug 8, 2016
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@cjihrig cjihrig mentioned this pull request Aug 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants