Skip to content

src: remove function hasTextDecoder in encoding.js - #23625

Closed
chichiwang wants to merge 2 commits into
nodejs:masterfrom
chichiwang:remove-internal-encoding-hasTextDecoder
Closed

src: remove function hasTextDecoder in encoding.js#23625
chichiwang wants to merge 2 commits into
nodejs:masterfrom
chichiwang:remove-internal-encoding-hasTextDecoder

Conversation

@chichiwang

@chichiwangchichiwang commented Oct 12, 2018•
edited
Loading

Copy link
Copy Markdown
Contributor

Remove hasTextDecoder in /lib/internal/encoding.js. hasTextDecoder is unused. This has the benefit of increasing test coverage.

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

@nodejs-github-botnodejs-github-bot added the encoding Issues and PRs related to the TextEncoder and TextDecoder APIs. label Oct 12, 2018
@addaleax

addaleax commented Oct 12, 2018

Copy link
Copy Markdown
Member

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

ChALkeR
ChALkeR approved these changes Oct 12, 2018
View reviewed changes

@ChALkeRChALkeR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@addaleaxaddaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 12, 2018
@mscdex

mscdex commented Oct 12, 2018

Copy link
Copy Markdown
Contributor

Can we also get rid of hasConverter (the function) entirely and then return the constructor directly?

@chichiwang

chichiwang commented Oct 12, 2018•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@mscdex It appears that hasConverter is used as a sort of encoding validator in makeTextDecoderJS but is unused in makeTextDecoderICU. We can probably remove it from makeTextDecoderICU.

It appears we can return the constructor directly from both TextDecoder factories. I'll make the change.

@Trott

Trott commented Oct 13, 2018

Copy link
Copy Markdown
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/1​77​91

@Trott

Trott commented Oct 13, 2018

Copy link
Copy Markdown
Member

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/1​7805

@Trott

Trott commented Oct 13, 2018

Copy link
Copy Markdown
Member

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/17817/

@danbev

danbev commented Oct 16, 2018

Copy link
Copy Markdown
Contributor

Re-run of failing node-test-commit-linux

@Trott

Trott commented Oct 16, 2018

Copy link
Copy Markdown
Member

The test-buffer-alloc failures will (I believe) be fixed by rebasing against master, which a Resume Build should do. Let's see....

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/17887/

@chichiwang

chichiwang commented Oct 16, 2018

Copy link
Copy Markdown
ContributorAuthor

@Trott Thanks! There still appears to be one failure in the run. Let me know if I can do anything to help resolve the CI failures (like rebase the branch against master).

@refack

refack commented Oct 16, 2018

Copy link
Copy Markdown
Contributor

Re-resume: https://ci.nodejs.org/job/node-test-pull-request/17912/

@jasnelljasnell self-assigned this Oct 16, 2018
@jasnell

jasnell commented Oct 16, 2018

Copy link
Copy Markdown
Member

Getting this landed.

jasnell pushed a commit that referenced this pull request Oct 16, 2018
@chichiwang@jasnell
src: remove function hasTextDecoder in encoding.js
25dc25b
also... return TextDecoder directly from factories
PR-URL: #23625
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@jasnell

jasnell commented Oct 16, 2018

Copy link
Copy Markdown
Member

Landed in 25dc25b

@jasnelljasnell closed this Oct 16, 2018
jasnell pushed a commit that referenced this pull request Oct 17, 2018
@chichiwang@jasnell
src: remove function hasTextDecoder in encoding.js
2f6b737
also... return TextDecoder directly from factories
PR-URL: #23625
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
addaleax pushed a commit that referenced this pull request Oct 20, 2018
@chichiwang@addaleax
src: remove function hasTextDecoder in encoding.js
8b431ad
also... return TextDecoder directly from factories
PR-URL: #23625
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@addaleaxaddaleax mentioned this pull request Oct 27, 2018
Closed
3 tasks
codebytere pushed a commit that referenced this pull request Dec 13, 2018
@chichiwang@codebytere
src: remove function hasTextDecoder in encoding.js
91b8c5b
also... return TextDecoder directly from factories
PR-URL: #23625
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 26, 2018
@chichiwang@MylesBorins
src: remove function hasTextDecoder in encoding.js
ffb4087
also... return TextDecoder directly from factories
PR-URL: #23625
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@codebyterecodebytere mentioned this pull request Jan 4, 2019
Merged
This was referenced Jan 29, 2019
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ChALkeRChALkeRChALkeR approved these changes
@jasnelljasnelljasnell approved these changes
@addaleaxaddaleaxaddaleax approved these changes
@cjihrigcjihrigcjihrig approved these changes
+1 more reviewer
@thefourtheyethefourtheyethefourtheye approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@jasnelljasnell

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.encodingIssues and PRs related to the TextEncoder and TextDecoder APIs.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

12 participants

@chichiwang@addaleax@mscdex@Trott@danbev@refack@jasnell@ChALkeR@thefourtheye@cjihrig@codebytere@nodejs-github-bot