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

stream: proposal to improve err msg when ._write is not implemented #7396

Closed
italoacasas opened this issue Jun 24, 2016 · 5 comments
Closed
Labels
feature request Issues that request new features to be added to Node.js. stream Issues and PRs related to the stream subsystem.

Comments

@italoacasas
Copy link
Contributor

Right now when someone try to use writable.write we don't give too much feedback in the error msg. I think we can do better on that area.

Example:

const ws = require('stream').Writable();
ws.write('example')

Output:

image 2016-06-23 at 10 41 19 pm

Proposal:

image 2016-06-23 at 10 35 38 pm

@mscdex mscdex added stream Issues and PRs related to the stream subsystem. feature request Issues that request new features to be added to Node.js. labels Jun 24, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Jun 24, 2016

This seems like too much. The existing error is not really helpful. What about something simpler like _write() must be implemented or _write() is not implemented. If you are creating your own writable stream, you should already be familiar with the docs.

@italoacasas
Copy link
Contributor Author

italoacasas commented Jun 24, 2016

I understand, but I think we have a lot of places that we can improve the error msg, maybe the format that I'm proposing, in this case, is not the best one but we should try to improve this kind of errors msg.

We cannot assume that all the developer are seniors, and in most cases, people don't read all the documentation, they read some random article in google. I don't think that giving more information like a link pointing to the specific documentation, a more readable error, etc... is not going to affect in a negative way on comprehension.

For newcomers is going to be something nice to have too, I would love to give this a shot, maybe I can make a proposal with different Ideas of how we can improve in my opinion this kind of errs msg where we have more information that we can show.

@cjihrig
Copy link
Contributor

cjihrig commented Jun 24, 2016

Sorry, but I'm -1 to elaborate error messages, and very -1 to including URLs in error messages.

@italoacasas
Copy link
Contributor Author

Yeah, on second thought I do not think a URL is a good idea. But I cannot see the downside in elaborate more the msg when we have more information that we can show.

@italoacasas
Copy link
Contributor Author

80% of the time when a person get this kind of error Error: not implemented they are going to use google to find the response, we can kill that step if we elaborate more the error msg.

ramisra pushed a commit to ramisra/node that referenced this issue Jul 12, 2016
…eam write method

Files changes include the fix for  issue
Fixes: nodejs#7396
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants