FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

errors,stream-transform: migrate to use internal/errors.js by sreepurnajasti · Pull Request #13310 · nodejs/node · GitHub

/ node Public

errors,stream-transform: migrate to use internal/errors.js - #13310

Closed
sreepurnajasti wants to merge 1 commit into
nodejs:masterfrom
sreepurnajasti:stream-transform
ConversationCommits (1)ChecksFiles changed
Closed

errors,stream-transform: migrate to use internal/errors.js#13310
sreepurnajasti wants to merge 1 commit into
nodejs:masterfrom
sreepurnajasti:stream-transform

Conversation

sreepurnajasti commented May 30, 2017

Copy link
Copy Markdown
Contributor
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

lib/_stream_transform.js

ref: #11273

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. stream Issues and PRs related to the stream subsystem. labels May 30, 2017
kunalspathak reviewed May 30, 2017
View reviewed changes
Comment thread lib/_stream_transform.js Outdated

kunalspathak May 30, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

'Calling transform done when still transforming' [](start = 27, length = 48)

don't need this anymore?

sreepurnajasti May 31, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@kunalspathak Thanks. It's done.

kunalspathak reviewed May 30, 2017
View reviewed changes
Comment thread test/parallel/test-stream-transform-callback-twice.js Outdated

kunalspathak May 30, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

[](start = 22, length = 1)

nit: can you move the space on above line?

sreepurnajasti May 31, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@kunalspathak Done.

jasnell reviewed May 30, 2017
View reviewed changes
Comment thread lib/internal/errors.js Outdated

jasnell May 30, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

If I recall correctly, there are a couple of places in core with a similar error. A more generic error message may be appropriate.

sreepurnajasti May 31, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@jasnell Modified the error message to be more generic. Thanks

kunalspathak commented May 31, 2017

Copy link
Copy Markdown
Member

@sreepurnajasti - Changes incorporated from my feedback looks good although I kind of don't like ERR_CALLING_WS_LENGTH as the reader doesn't easily get the intent of the error message. I will leave it to @jasnell to comment on this.

mhdawson reviewed May 31, 2017
View reviewed changes
Comment thread lib/_stream_transform.js Outdated

mhdawson May 31, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'd agree a better name for the error would be good. Maybe 'ERR_TRANSFORM_WITH_LENGTH_0'.

sreepurnajasti Jun 13, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@mhdawson Done

Comment thread lib/_stream_transform.js Outdated

mhdawson May 31, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Maybe to be consistent with my other suggestion:
'ERR_TRANSFORM_MULTIPLE_CALLBACK'.

sreepurnajasti Jun 13, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@mhdawson Fixed.

Comment thread lib/_stream_transform.js Outdated

mhdawson May 31, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Maybe to be consistent with my other suggestion:
'ERR_TRANSFORM_ALREADY_TRANSFORMING' String is already specific to transform so making that part of the ID makes sense to me.

sreepurnajasti Jun 13, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@mhdawson Done

sreepurnajasti commented Jun 12, 2017

Copy link
Copy Markdown
Contributor Author

@mhdawson @jasnell As per the suggestions, it is modified. Please, have a look.

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 13, 2017
sreepurnajasti force-pushed the stream-transform branch 2 times, most recently from 47ada2c to 0cc5f2d Compare June 14, 2017 14:28

mhdawson commented Jun 14, 2017

Copy link
Copy Markdown
Member

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

mhdawson commented Jun 15, 2017

Copy link
Copy Markdown
Member

CI good landing

mhdawson commented Jun 15, 2017

Copy link
Copy Markdown
Member

Landed as d50a802

mhdawson pushed a commit that referenced this pull request Jun 15, 2017
errors,stream-transform: migrate to use internal/errors.js
d50a802 
PR-URL: #13310
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
TimothyGu closed this Jun 15, 2017

sreepurnajasti commented Jun 18, 2017

Copy link
Copy Markdown
Contributor Author

@mhdawson Thank you :)

mcollina commented Jun 21, 2017

Copy link
Copy Markdown
Member

I'm strongly -1 on this one. This forces some good rework on readable-stream to pull those error data. May I ask for a revert?

cc @nodejs/streams

mcollina commented Jun 21, 2017

Copy link
Copy Markdown
Member

To be clear, this is something we should be doing asap, but we need to think a bit how we want to do it.

jasnell commented Jun 21, 2017

Copy link
Copy Markdown
Member

:-/ ... reverting would be unfortunate. Perhaps instead, since this is semver-major and won't go out in a release any time soon, can we take a short bit of time to figure out the strategy for readable-stream? And if we can't identify a reasonable path forward, then revert...

mcollina commented Jun 21, 2017

Copy link
Copy Markdown
Member

That's ok for me, there is no hurry to revert.

jasnell mentioned this pull request Oct 30, 2017
Merged
sreepurnajasti deleted the stream-transform branch December 27, 2017 06:36
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
+2 more reviewers
kunalspathak kunalspathak left review comments
mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

errors Issues and PRs related to JavaScript errors originated 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 the stream subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL