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.

Sign up for GitHub

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

Jump to bottom

errorMessage: Improvement in the error thrown in case of writable st… #7671

Closed
wants to merge 3 commits into from
Closed

errorMessage: Improvement in the error thrown in case of writable st… #7671

wants to merge 3 commits into from

Conversation

ramisra
Copy link
Contributor

@ramisra ramisra commented Jul 12, 2016 •
edited

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

Write stream _stream_writable.js is the file that has been changed

Description of change

…ream write method

Files changes include the fix for issue
Fixes: #7396

errorMessage: Improvement in the error thrown in case of writable str…
02924b0 
…eam write method

Files changes include the fix for  issue
Fixes: nodejs#7396
@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Jul 12, 2016
@mscdex
Copy link
Contributor

mscdex commented Jul 12, 2016

I'm confused, this doesn't improve the message, it just removes it. If that was intentional, I don't that is the ideal solution.

@cjihrig
Copy link
Contributor

cjihrig commented Jul 12, 2016

Agreed. This commit also leaves commented code behind, and adds unnecessary whitespace lines.

@ramisra
Copy link
Contributor Author

ramisra commented Jul 12, 2016

i was thinking to catch the error and return some logical comment but at last commented the error partas it was not making sense . What would be a possible solution for this @cjihrig

@mscdex
Copy link
Contributor

mscdex commented Jul 12, 2016

@ratikesh9 I think the original suggestion was to improve the message string itself to be perhaps a little more descriptive, not to change the actual logic/behavior.

@ramisra
Copy link
Contributor Author

ramisra commented Jul 12, 2016

Can the error message in this 741d2fe commit would be a sufficient fix ?

cjihrig
cjihrig reviewed Jul 12, 2016
View reviewed changes
lib/_stream_writable.js
@@ -432,7 +434,7 @@ function clearBuffer(stream, state) {
}

Writable.prototype._write = function(chunk, encoding, cb) {
cb(new Error('not implemented'));
cb(new Error('Write method cannot be invoked on the Writable stream . Method not implemented'));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this could be as simple as _write() must be implemented or _write() is not implemented.

@ramisra
Copy link
Contributor Author

ramisra commented Jul 12, 2016

cool will remove this

@ramisra
Whitespaces removed + Message fix
c2108c7 
* Whitespaces comprising line 200, 309 removed .
* Error Message changed under _write function .
@cjihrig
Copy link
Contributor

cjihrig commented Jul 12, 2016

I'd put () on the end of _write, but otherwise, LGTM. Let's see what others think.

@evanlucas
Copy link
Contributor

I'm +0 on it. I do think it should be semver major though since it is changing the error message

@addaleax addaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 13, 2016
@jasnell
Copy link
Member

jasnell commented Jul 20, 2016 •
edited

I'm fairly -1 on this as it currently stands. I tend to prefer my error messages to be a bit more descriptive and I'm not seeing the value in this particular change.
Scratch that... in my vacation fogged brain I totally read that diff backwards... sigh... lol :-)
LGTM!

@ramisra
Copy link
Contributor Author

ramisra commented Jul 21, 2016

@jasnell Dude you just though made me upset with your initial comment. 👍

@cjihrig
Copy link
Contributor

cjihrig commented Jul 21, 2016

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

@cjihrig
Copy link
Contributor

cjihrig commented Jul 21, 2016

CI had one unrelated failure. Landing this.

@cjihrig
Copy link
Contributor

cjihrig commented Jul 21, 2016

Landed in 9983af0. Thanks!

@cjihrig cjihrig closed this Jul 21, 2016
cjihrig pushed a commit that referenced this pull request Jul 21, 2016
@ramisra @cjihrig
stream: improve unimplemented _write() error
9983af0 
This commit improves the ambiguous "not implemented" error
that is provided when a writable stream does not implement
_write().

Fixes: #7396
PR-URL: #7671
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
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.

stream: proposal to improve err msg when ._write is not implemented
7 participants
@ramisra @mscdex @cjihrig @evanlucas @jasnell @addaleax @nodejs-github-bot