Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.3k
[buffer]: introduce the 'latin1' encoding string#7111
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
trevnorris commented Jun 2, 2016 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Fishrock123 commented Jun 2, 2016
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 commented Jun 2, 2016
LGTM |
jasnell commented Jun 2, 2016
semver-major because of the docs-only deprecation of |
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 commented Jun 2, 2016 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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 |
addaleax commented Jun 2, 2016
I don’t think anybody’s expecting to remove |
trevnorris commented Jun 2, 2016 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
@jasnell didn't realize doc only deprecation would be semver-major. if that's the case can I just remove edit: to clarify, reason for deprecation is as @addaleax pointed out. to simply point out that |
jasnell commented Jun 2, 2016
All other doc only deprecations we've done have been treated as semver-majors as well. |
trevnorris commented Jun 3, 2016
@jasnell There you go. Changed documentation to state that |
trevnorris commented Jun 7, 2016 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
@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 commented Jun 7, 2016
Lgtm |
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]>
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 commented Jun 7, 2016
bnoordhuis commented Jun 17, 2016
I'm changing this to semver-major again because the change to the It could land in concert with #7284 as a semver-minor change but honestly, I'd just wait until v7. |
trevnorris commented Jun 17, 2016
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 commented Jun 22, 2016
I’d be +1 for landing these together in a semver-minor, too. |
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]>
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]>
Affected core subsystem(s)
buffer,string_bytesDescription 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 encodingstring was introduced.
Introduce the new encoding string
'latin1'to be more explicit. Theprevious
'binary'is still around and only deprecated in documentation.While many tests have switched to use
'latin1', there are still plentythat do both
'binary'and'latin1'checks side-by-side to ensure thereis 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/