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

src: change constructor behavior in stream_base-inl.h #23447

Closed
wants to merge 2 commits into from
Closed

src: change constructor behavior in stream_base-inl.h #23447

wants to merge 2 commits into from

Conversation

iansu
Copy link
Contributor

@iansu iansu commented Oct 12, 2018 •
edited
Loading

Change ConstructorBehavior from kAllow to kThrow.

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

@iansu
src: change constructor behavior in stream_base-inl.h
68fba54 
Change ConstructorBehavior from kAllow to kThrow.
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 12, 2018
@gireeshpunathil gireeshpunathil added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 12, 2018
@addaleax
Copy link
Member

@iansu Can you also remove the TODO comment itself, now that it’s done? :)

@addaleax addaleax mentioned this pull request Oct 12, 2018
Closed
2 tasks
@addaleax
Copy link
Member

@iansu This PR is very similar to #23453 – that’s my fault. One of the tasks we handed out had text pointing to the same file, even though the heading pointed to another file (node_crypto.cc).

I’ll try to land both of these PRs together so you both get full credit, but just a heads up, if you have the test with the heading mentioning node_crypto.cc, feel free to also do that! Sorry again. :(

@addaleax addaleax self-assigned this Oct 12, 2018
@iansu
Copy link
Contributor Author

iansu commented Oct 12, 2018

@addaleax I can fix this in node_crypto.cc if it hasn't been done already. Should I do that here or in a new PR?

@addaleax
Copy link
Member

@iansu We have #23582 for that – sorry for the confusion, should be all good now!

@Trott
Copy link
Member

Trott commented Oct 14, 2018

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

@addaleax
Copy link
Member

Landed in c19ab56

Thanks for the contribution! 🎉

(If you're interested in other possible contributions to Node.js but don't have a good idea of where to start looking, some ideas are posted at https://www.nodetodo.org/next-steps/.)

@addaleax addaleax closed this Oct 14, 2018
addaleax pushed a commit that referenced this pull request Oct 14, 2018
@iansu @bmacnaughton @addaleax
src: change constructor behavior in stream_base-inl.h
c19ab56 
Change ConstructorBehavior from kAllow to kThrow.

Co-authored-by: Bruce A. MacNaughton <[email protected]>
Refs: #23453
PR-URL: #23447
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
@iansu @bmacnaughton @jasnell
src: change constructor behavior in stream_base-inl.h
ec7375a 
Change ConstructorBehavior from kAllow to kThrow.

Co-authored-by: Bruce A. MacNaughton <[email protected]>
Refs: #23453
PR-URL: #23447
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@tniessen tniessen tniessen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

Assignees

@addaleax addaleax

Labels
c++ Issues and PRs that require attention from people who are familiar with C++. code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@iansu @addaleax @Trott @jasnell @cjihrig @tniessen @gireeshpunathil @nodejs-github-bot