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

doc: fix encoding string in buffer example #12482

Closed
wants to merge 1 commit into from
Closed

doc: fix encoding string in buffer example #12482

wants to merge 1 commit into from

Conversation

MapleGu
Copy link
Contributor

@MapleGu MapleGu commented Apr 18, 2017

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations. labels Apr 18, 2017
@jseijas
Copy link

jseijas commented Apr 18, 2017

Hi! In the write method of Buffer there is a switch for the encoding, where the different encoding are taked into account with dash and without dash. So bot utf8 and utf-8 works, ucs2 and ucs-2, utf16le and utf-16le... I think that the best solution is not to modify the documentation, but adding the latin-1 option in the "case 7" of the switch:

 case 7:
      if (encoding === 'utf16le' || encoding.toLowerCase() === 'utf16le')
        return this.ucs2Write(string, offset, length);
      /* begin modification */
      if (encoding === 'latin-1')
        return this.latin1Write(string, offset, length);
      /* end modification */
      break;
    case 8:
      if (encoding === 'utf-16le' || encoding.toLowerCase() === 'utf-16le')
        return this.ucs2Write(string, offset, length);
      break;
    case 6:
      if (encoding === 'latin1' || encoding === 'binary')
        return this.latin1Write(string, offset, length);

@vsemozhetbyt
Copy link
Contributor

cc @nodejs/buffer

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Thank you! It would be great if you could reword the commit message so that it starts with doc: and describes what the patch itself does, e.g. doc: fix encoding string in buffer example. If you don’t feel comfortable doing that we can also do that for you when landing the commit. :)

@addaleax
Copy link
Member

@jseijas I would be careful about adding another encoding alias; the ones we have right now are mostly there for historic reasons, and having to support more encoding strings comes with a certain performance impact.

That we have introduced latin1 as an alias of binary in the first place shows that there are obviously exceptions, of course. ;)

(Also, there’s quite a few more places in the codebase where such an alias would have to be supported.)

@jasnell
Copy link
Member

jasnell commented Apr 18, 2017

+1 to @addaleax here... adding a new alias carries a definite non-zero cost in terms of complexity and performance for very little gain.

@jasnell
Copy link
Member

jasnell commented Apr 18, 2017

The doc change is trivial enough that this shouldn't need to wait 48 hours to land

@TimothyGu
Copy link
Member

Landed in 3bf358d.

@TimothyGu TimothyGu closed this Apr 18, 2017
TimothyGu pushed a commit that referenced this pull request Apr 18, 2017
PR-URL: #12482
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MapleGu MapleGu changed the title NodeJS v7.9.0 does not support latin-1 doc: fix encoding string in buffer example Apr 19, 2017
evanlucas pushed a commit that referenced this pull request Apr 25, 2017
PR-URL: #12482
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@evanlucas evanlucas mentioned this pull request May 1, 2017
evanlucas pushed a commit that referenced this pull request May 1, 2017
PR-URL: #12482
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
evanlucas pushed a commit that referenced this pull request May 2, 2017
PR-URL: #12482
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
gibfahn pushed a commit that referenced this pull request May 16, 2017
PR-URL: #12482
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 18, 2017
PR-URL: #12482
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 23, 2017
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
PR-URL: nodejs/node#12482
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
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. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants