Skip to content

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

Closed
sreepurnajasti wants to merge 1 commit intonodejs:masterfrom
sreepurnajasti:stream-transform
Closed

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

Conversation

@sreepurnajasti
Copy link
Contributor

@sreepurnajasti sreepurnajasti commented May 30, 2017

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 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
kunalspathak reviewed May 30, 2017
View reviewed changes
lib/_stream_transform.js Outdated
Copy link
Member

@kunalspathak kunalspathak May 30, 2017

Choose a reason for hiding this comment

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

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

don't need this anymore?

Copy link
Contributor Author

@sreepurnajasti sreepurnajasti May 31, 2017

Choose a reason for hiding this comment

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

@kunalspathak Thanks. It's done.

kunalspathak
kunalspathak reviewed May 30, 2017
View reviewed changes
test/parallel/test-stream-transform-callback-twice.js Outdated
Copy link
Member

@kunalspathak kunalspathak May 30, 2017

Choose a reason for hiding this comment

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

[](start = 22, length = 1)

nit: can you move the space on above line?

Copy link
Contributor Author

@sreepurnajasti sreepurnajasti May 31, 2017

Choose a reason for hiding this comment

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

@kunalspathak Done.

jasnell
jasnell reviewed May 30, 2017
View reviewed changes
lib/internal/errors.js Outdated
Copy link
Member

@jasnell jasnell May 30, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@sreepurnajasti sreepurnajasti May 31, 2017

Choose a reason for hiding this comment

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

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

@kunalspathak
Copy link
Member

kunalspathak commented May 31, 2017

@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
mhdawson reviewed May 31, 2017
View reviewed changes
lib/_stream_transform.js Outdated
Copy link
Member

@mhdawson mhdawson May 31, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@sreepurnajasti sreepurnajasti Jun 13, 2017

Choose a reason for hiding this comment

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

@mhdawson Done

lib/_stream_transform.js Outdated
Copy link
Member

@mhdawson mhdawson May 31, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@sreepurnajasti sreepurnajasti Jun 13, 2017

Choose a reason for hiding this comment

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

@mhdawson Fixed.

lib/_stream_transform.js Outdated
Copy link
Member

@mhdawson mhdawson May 31, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@sreepurnajasti sreepurnajasti Jun 13, 2017

Choose a reason for hiding this comment

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

@mhdawson Done

@sreepurnajasti
Copy link
Contributor Author

sreepurnajasti commented Jun 12, 2017

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

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

mhdawson commented Jun 14, 2017

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

@mhdawson
Copy link
Member

mhdawson commented Jun 15, 2017

CI good landing

@mhdawson
Copy link
Member

mhdawson commented Jun 15, 2017

Landed as d50a802

mhdawson pushed a commit that referenced this pull request Jun 15, 2017
@mhdawson
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 TimothyGu closed this Jun 15, 2017
@sreepurnajasti
Copy link
Contributor Author

sreepurnajasti commented Jun 18, 2017

@mhdawson Thank you :)

@mcollina
Copy link
Member

mcollina commented Jun 21, 2017

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

mcollina commented Jun 21, 2017

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

jasnell commented Jun 21, 2017

:-/ ... 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
Copy link
Member

mcollina commented Jun 21, 2017

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

@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
@sreepurnajasti sreepurnajasti deleted the stream-transform branch December 27, 2017 06:36
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

@mhdawson mhdawson mhdawson approved these changes

+1 more reviewer

@kunalspathak kunalspathak kunalspathak left review comments

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.

7 participants

@sreepurnajasti @kunalspathak @mhdawson @mcollina @jasnell @TimothyGu @nodejs-github-bot