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: improving error msg for stream #8801

Closed
wants to merge 4 commits into from
Closed

stream: improving error msg for stream #8801

wants to merge 4 commits into from

Conversation

italoacasas
Copy link
Contributor

@italoacasas italoacasas commented Sep 27, 2016

Checklist
  • make -j8 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)
  • stream
Description of change
  • Improving error msg when Transform._transform() is not implement
  • Improving error msg when Readable._read() is not implement
  • Removing extra word for error msg when Writable._write() is not implemented

@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Sep 27, 2016
@Trott Trott added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 27, 2016
@Trott
Copy link
Member

Trott commented Sep 27, 2016

I think we're still in a condition where (unfortunately) changing an error message means the change is semver major. (/cc @jasnell in case I'm wrong) Labeling semver-major.

Change looks good to me, although I would probably be inclined to omit the word method since the () makes it clear that it's a function in the first place. But that's totally a nit you can ignore.

@ronkorving
Copy link
Contributor

ronkorving commented Sep 27, 2016

I don't care too much about "method" being there or not, but I'm happy with this change 👍 (and agree it's semver-major)

@italoacasas
Copy link
Contributor Author

italoacasas commented Sep 27, 2016

@Trott my first through was _transform() is not implemented but then I see this on _writable https://github.com/nodejs/node/blob/master/lib/_stream_writable.js#L435, I just trying to maintain consistency. Maybe the correct solution is to fix _writable instead of adding method here.

@lpinca
Copy link
Member

lpinca commented Sep 27, 2016

+1 for using consistent error messages. It's semver major so changing _write error message could also be an option.

@italoacasas italoacasas changed the title stream: improving msg when transform._transform() method is not implemented stream: improving error msg for stream Sep 27, 2016
@italoacasas
Copy link
Contributor Author

italoacasas commented Sep 27, 2016

I just remove method from Writable._write() and add the same error msg in Readable._read()


# Visual Studio Code
.vscode/
jsconfig.json
Copy link
Contributor

Choose a reason for hiding this comment

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

could you please add a newline at the end here?

Copy link
Member

Choose a reason for hiding this comment

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

Do we actually want to add that data to .gitignore? If I'm not wrong there is an ongoing discussion in another PR for .idea and another open PR to whitelist stuff instead of blacklisting.
I think .idea, .vscode and the others should be in the user global gitignore not here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found the PR: #6963

... I think is just better to add this kind of common files to .gitignore that asking people to add this to the global ignore. Just because this way we don't need to mention again something about this topic and really this don't hurt anyone

Copy link
Member

Choose a reason for hiding this comment

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

@italoacasas #8016 is a much better option imho.

@jasnell
Copy link
Member

jasnell commented Sep 27, 2016

LGTM with nits addressed and green CI.

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM without the .gitignore changes.

@@ -93,3 +92,7 @@ test.tap
# Xcode workspaces and project folders
*.xcodeproj
*.xcworkspace

# Visual Studio Code
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be done elsewhere.

@italoacasas
Copy link
Contributor Author

Done

@lpinca
Copy link
Member

lpinca commented Sep 27, 2016

LGTM

Copy link
Member

@imyller imyller left a comment

Choose a reason for hiding this comment

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

LGTM

@imyller imyller self-assigned this Sep 29, 2016
@imyller
Copy link
Member

imyller commented Sep 29, 2016

@imyller
Copy link
Member

imyller commented Sep 29, 2016

I'll start landing this:

  • Five LGTMs
  • No objections
  • Requested modifications have been made
  • CI tests passed

imyller pushed a commit that referenced this pull request Sep 29, 2016
Improve message when tranform._transform() method is not implemented
Improve error message when Readable._read() is not implemented
Remove extra word in err msg when Writable._write() when not implemented
Remove extra word in err msg when Transform._transform() when not implemented

PR-URL: #8801
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
@imyller
Copy link
Member

imyller commented Sep 29, 2016

Landed in 560a589

Thank you for your effort and contribution, @italoacasas

@imyller imyller closed this Sep 29, 2016
@imyller imyller removed their assignment Sep 29, 2016
@jasnell
Copy link
Member

jasnell commented Oct 10, 2016

@nodejs/ctc ... any objections to this landing in v7.x?

jasnell pushed a commit that referenced this pull request Oct 10, 2016
Improve message when tranform._transform() method is not implemented
Improve error message when Readable._read() is not implemented
Remove extra word in err msg when Writable._write() when not implemented
Remove extra word in err msg when Transform._transform() when not implemented

PR-URL: #8801
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

9 participants