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

errors,_stream_wrap: change _stream_wrap.js to use internal/errors.js #13291

Closed
wants to merge 1 commit into from
Closed

errors,_stream_wrap: change _stream_wrap.js to use internal/errors.js #13291

wants to merge 1 commit into from

Conversation

@LakshmiSwethaG
Copy link
Contributor

@LakshmiSwethaG LakshmiSwethaG commented May 30, 2017

Refs: #11273

@jasnell, pinging you for mentoring as this is my first PR to this project, thank you!

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

_stream_wrap.js

@@ -49,7 +50,8 @@ function StreamWrap(stream) {
this.pause();
this.removeListener('data', ondata);

self.emit('error', new Error('Stream has StringDecoder'));
self.emit('error', new errors.Error('ERR_ASSERTION',
Copy link
Member

@jasnell jasnell Jun 1, 2017

Choose a reason for hiding this comment

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

ERR_ASSERTION is the wrong code to use here. That should only be used with the assert module. I recommend creating a new code specific to this condition... perhaps ERR_STREAM_HAS_STRINGDECODER

Loading

Copy link
Contributor Author

@LakshmiSwethaG LakshmiSwethaG Jun 5, 2017

Choose a reason for hiding this comment

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

@jasnell , I followed your suggestions and made those changes.
In this process, I made few mistakes but I guess I recovered everything, please have a look.
Thanks.

Loading

@@ -49,7 +50,8 @@ function StreamWrap(stream) {
this.pause();
this.removeListener('data', ondata);

self.emit('error', new Error('Stream has StringDecoder'));
self.emit('error', new errors.Error('ERR_STREAM_HAS_STRINGDECODER',
'Stream has StringDecoder'));
Copy link
Member

@jasnell jasnell Jun 5, 2017

Choose a reason for hiding this comment

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

Just move the message to the internal/errors.js file where the code is declared, then simplify this to just:

self.emit('error', new errors.Error('ERR_STREAM_HAS_STRINGDECODER'));

Loading

@mhdawson
Copy link
Member

@mhdawson mhdawson commented Jun 6, 2017

Unfortunately this needs a rebase.

Loading

@LakshmiSwethaG
Copy link
Contributor Author

@LakshmiSwethaG LakshmiSwethaG commented Jun 7, 2017

Done the rebase and resolved the conflicts. Please review it. Thanks

Loading

jasnell
jasnell approved these changes Jun 9, 2017
@mhdawson
Copy link
Member

@mhdawson mhdawson commented Jun 16, 2017

Loading

@tniessen tniessen self-assigned this Jun 17, 2017
tniessen added a commit that referenced this issue Jun 20, 2017
PR-URL: #13291
Refs: #11273
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@tniessen
Copy link
Member

@tniessen tniessen commented Jun 20, 2017

Landed in d291338, thank you for your contribution! 🎉

Loading

@addaleax
Copy link
Member

@addaleax addaleax commented Jun 20, 2017

Labelled this semver-major as I think it should have been (not that there’s anything wrong with the patch), if I’m wrong feel free to remove the label.

Loading

@tniessen
Copy link
Member

@tniessen tniessen commented Jun 20, 2017

@addaleax The error message did not change, why would this be semver-major?

Loading

@addaleax
Copy link
Member

@addaleax addaleax commented Jun 20, 2017

@tniessen The error message changed from Error: Stream has StringDecoder to Error [ERR_STREAM_HAS_STRINGDECODER]: Stream has StringDecoder

Loading

@tniessen
Copy link
Member

@tniessen tniessen commented Jun 20, 2017

@addaleax Right, my bad.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants