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

src: changed stdio_pipes_ to std::vector #23615

Merged
merged 1 commit into from
Oct 17, 2018
Merged

Conversation

niboch
Copy link
Contributor

@niboch niboch commented Oct 12, 2018

Checklist
  • [ x] make -j4 test (UNIX), or vcbuild test (Windows) passes
  • [ x] commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. child_process Issues and PRs related to the child_process subsystem. labels Oct 12, 2018
if (h != nullptr) {
r = h->Start();
std::vector<std::unique_ptr<SyncProcessStdioPipe>>::iterator iter;
for (iter = stdio_pipes_.begin(); iter != stdio_pipes_.end(); ++iter) {
Copy link
Member

Choose a reason for hiding this comment

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

Feel free to use a for (const auto& pipe : stdio_pipes_) loop here :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to explicitly request this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

FTR here are some of the guidelines I'm basing my request on:
ES.55: Avoid the need for range checking
ES.6: Declare names in for-statement initializers and conditions to limit scope
ES.11: Use auto to avoid redundant repetition of type names
ES.20: Always initialize an object

stdio_pipes_[i]->Close();
std::vector<std::unique_ptr<SyncProcessStdioPipe>>::iterator iter;
for (iter = stdio_pipes_.begin(); iter != stdio_pipes_.end(); ++iter) {
SyncProcessStdioPipe* pipe = (*iter).get();
Copy link
Member

Choose a reason for hiding this comment

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

ditto here :)

@BridgeAR BridgeAR 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

@addaleax
Copy link
Member

refack
refack previously requested changes Oct 16, 2018
if (h != nullptr) {
r = h->Start();
std::vector<std::unique_ptr<SyncProcessStdioPipe>>::iterator iter;
for (iter = stdio_pipes_.begin(); iter != stdio_pipes_.end(); ++iter) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to explicitly request this change.

@refack
Copy link
Contributor

refack commented Oct 16, 2018

Hello @niboch and thank you for the contribution 🥇
I've requested a small change in the code in order for it to comply with our C++ guidelines. I hope you have the time to follow up on this.

@refack refack self-assigned this Oct 16, 2018
@BridgeAR
Copy link
Member

@refack this can also be resolved while landing. I'll put together a PR that resolves that later on.

@BridgeAR
Copy link
Member

I just went ahead and pushed a fixup to the branch and dismissed the comment due to that.

@BridgeAR BridgeAR dismissed refack’s stale review October 17, 2018 08:59

Comment addressed.

@addaleax
Copy link
Member

addaleax commented Oct 17, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/17934/ (:heavy_check_mark:)

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 17, 2018
@refack
Copy link
Contributor

refack commented Oct 17, 2018

I'm actually of a mind to say that if the author in not engaged, and doesn't follow up, we shouldn't land.
@niboch I do hope you are interested in following up on this.

refack
refack previously requested changes Oct 17, 2018
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Give author a change to follow up.

@refack refack removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 17, 2018
@refack
Copy link
Contributor

refack commented Oct 17, 2018

P.s. please don't dismiss my review without my consent.

@BridgeAR
Copy link
Member

@refack if you disagree with this, I suggest to open an issue to discuss this in general. But so far we have often helped people to get their PR into Node.js, no matter if we had to "fix" small edges or not (this is especially true for Code & Learn events). In this case the PR itself was technically fine and it's mainly a "style" issue. Thus, I don't think we should block this PR.

@jasnell
Copy link
Member

jasnell commented Oct 17, 2018

@refack ... I agree with @BridgeAR on this. From what I can see, the issues have been addressed on this and the PR should be ready to land.

@jasnell
Copy link
Member

jasnell commented Oct 17, 2018

Ping @nodejs/tsc ... please weigh in on this.

@refack
Copy link
Contributor

refack commented Oct 17, 2018

@refack if you disagree with this, I suggest to open an issue to discuss this in general.

Ack. I agree that it's a tangent moving to #23712

@Trott
Copy link
Member

Trott commented Oct 17, 2018

Ping @nodejs/tsc ... please weigh in on this.

Agree with @jasnell and @BridgeAR. The question is whether the change is an improvement to the code base, not who did or didn't do what and when. I'm opposed to permitting the blocking PRs on "author engagement" metrics.

That said, caring about author engagement is reasonable. I just don't think it should block a change that is ready to go or trivially close.

@refack
Copy link
Contributor

refack commented Oct 17, 2018

I say give the author time to follow up, don't do the work for them. The whole "edit others PRs" issue is a delicate one, and might even be experienced as condescending or aggressive by some.

tl;dr, let's restore the change set and allow time for the author to follow up.

@niboch
Copy link
Contributor Author

niboch commented Oct 17, 2018

Hey Everyone, Sorry about not responding earlier!

I had fixed it in my local code but did not push as I've been a bit busy at work, Since this is my first pull request I wasn't sure how to amend it and was going to ask how to do that here.

It looks like @BridgeAR already pushed So I assume everything is good? My code was basically the same as what he committed.

@refack
Copy link
Contributor

refack commented Oct 17, 2018

@niboch thank you for following up. I'm happy you are still engaged, and I hope we can land this soon.

@refack refack dismissed their stale review October 17, 2018 17:16

Got authors aproval

PR-URL: nodejs#23615
Reviewed-By: Sam Ruby <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@refack refack merged commit c979fad into nodejs:master Oct 17, 2018
@refack
Copy link
Contributor

refack commented Oct 17, 2018

🎉 Congratulations @niboch on getting promoted from
image
to
image

I hope you enjoyed the experience, and that we will see more contributions from you in the future 😉

@Trott
Copy link
Member

Trott commented Oct 17, 2018

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/.)

jasnell pushed a commit that referenced this pull request Oct 17, 2018
PR-URL: #23615
Reviewed-By: Sam Ruby <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
addaleax pushed a commit that referenced this pull request Oct 20, 2018
PR-URL: #23615
Reviewed-By: Sam Ruby <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@refack refack removed their assignment Oct 20, 2018
MylesBorins pushed a commit that referenced this pull request Oct 30, 2018
PR-URL: #23615
Reviewed-By: Sam Ruby <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
PR-URL: #23615
Reviewed-By: Sam Ruby <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
PR-URL: #23615
Reviewed-By: Sam Ruby <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. child_process Issues and PRs related to the child_process subsystem. code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants