GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — stream: complete migration to internal/errors by mcollina · Pull Request #16589 · nodejs/node

Conversation

@mcollina

@mcollina mcollina commented Oct 29, 2017

Copy link
Copy Markdown
Member
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)

stream

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mcollina
mcollina requested review from a team, BridgeAR and jasnell October 29, 2017 15:39
@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originating in Node.js core. stream Issues and PRs related to Node.js streams. labels Oct 29, 2017
@mcollina

mcollina commented Oct 29, 2017

Copy link
Copy Markdown
Member Author

@nodejs/tsc this needs expedite, as we need it for the 9 release. I'm sorry, but I caught these just now.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax
addaleax approved these changes Oct 29, 2017
View reviewed changes
@mcollina

mcollina commented Oct 29, 2017

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/11075/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax

addaleax commented Oct 29, 2017

Copy link
Copy Markdown
Member

I think @jasnell wanted to build the binaries ~ now? I’m okay with expediting but maybe we have the time for a CITGM run as well?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 29, 2017
jasnell
jasnell approved these changes Oct 29, 2017
View reviewed changes

@jasnell jasnell left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

One bit but generally lgtm and +1 on fast tracking

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/_stream_readable.js Outdated

@jasnell jasnell Oct 29, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Should probably just use ERR_INVALID_ARG_TYPE here

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/_stream_writable.js Outdated

@jasnell jasnell Oct 29, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

And here

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mcollina

mcollina commented Oct 29, 2017

Copy link
Copy Markdown
Member Author

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1031/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
joyeecheung
joyeecheung approved these changes Oct 29, 2017
View reviewed changes
Comment thread doc/api/errors.md Outdated

@joyeecheung joyeecheung Oct 29, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Missing link definitions, so are Writable and stream.write()

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mcollina mcollina Oct 29, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

added

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mcollina mcollina mentioned this pull request Oct 29, 2017
Proposal: 2017-10-31, Version 9.0.0 (Current) #15135
Merged
@mcollina
stream: complete migration to internal/errors
2651f29 
Complete the migration to the new error system of _stream_readable
and _stream_writable. Adds the corresponding documentation.
@mcollina
mcollina force-pushed the stream-new-errors branch from d464d69 to 2651f29 Compare October 29, 2017 16:09
@mcollina

mcollina commented Oct 29, 2017

Copy link
Copy Markdown
Member Author

@addaleax @joyeecheung @jasnell updated.

CI: https://ci.nodejs.org/job/node-test-pull-request/11077
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina

mcollina commented Oct 29, 2017

Copy link
Copy Markdown
Member Author

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1032/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Oct 29, 2017

Copy link
Copy Markdown
Member

I'll be doing one final pull from master this afternoon and will be kicking off the release build later on today.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina

mcollina commented Oct 29, 2017

Copy link
Copy Markdown
Member Author

Landed as 6e86a66
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina mcollina closed this Oct 29, 2017
@mcollina
mcollina deleted the stream-new-errors branch October 29, 2017 18:08
mcollina added a commit that referenced this pull request Oct 29, 2017
@mcollina
stream: complete migration to internal/errors
6e86a66 
Complete the migration to the new error system of _stream_readable
and _stream_writable. Adds the corresponding documentation.

PR-URL: #16589
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@maclover7 maclover7 mentioned this pull request Oct 29, 2017
doc: add link for stream.pipe() #16593
Closed
2 tasks done
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
@mcollina @Qard
stream: complete migration to internal/errors
b488fa8 
Complete the migration to the new error system of _stream_readable
and _stream_writable. Adds the corresponding documentation.

PR-URL: nodejs/node#16589
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
@mcollina @Qard
stream: complete migration to internal/errors
f470c31 
Complete the migration to the new error system of _stream_readable
and _stream_writable. Adds the corresponding documentation.

PR-URL: nodejs/node#16589
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@mcollina @addaleax
stream: complete migration to internal/errors
19fbfbd 
Complete the migration to the new error system of _stream_readable
and _stream_writable. Adds the corresponding documentation.

PR-URL: nodejs/node#16589
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@kjin kjin mentioned this pull request May 1, 2018
[8.x] Backport http2 changes from 9.x #20456
Closed
3 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@addaleax addaleax addaleax approved these changes
@joyeecheung joyeecheung joyeecheung approved these changes
@BridgeAR BridgeAR Awaiting requested review from BridgeAR

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

errors Issues and PRs related to JavaScript errors originating in Node.js core. semver-major PRs that contain breaking changes and should be released in the next major version. stream Issues and PRs related to Node.js streams.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@mcollina @addaleax @jasnell @joyeecheung @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.