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: remove duplicate env field from CryptoJob #31588

Open
wants to merge 2 commits into
base: master
from

Conversation

@bnoordhuis
Copy link
Member

bnoordhuis commented Jan 31, 2020

Alternative take on #31554 that tightens up the use of env() getters.

First:

src: remove duplicate env field from CryptoJob

Add a `ThreadPoolWork::env()` getter and use that. To fix the diamond
problem in class CompressionStream that inherits from both AsyncWrap
and ThreadPoolWork, we add a `CompressionStream::env()` getter that
delegates to `AsyncWrap::env()`.

And then:

src: StreamBase::stream_env() -> StreamBase::env()

Rename `StreamBase::stream_env()` to `StreamBase::env()` and work around
the diamond problem by adding `env()` getters to several classes.
bnoordhuis added 2 commits Jan 31, 2020
Rename `StreamBase::stream_env()` to `StreamBase::env()` and work around
the diamond problem by adding `env()` getters to several classes.

Refs: #31554 (comment)
Add a `ThreadPoolWork::env()` getter and use that. To fix the diamond
problem in class CompressionStream that inherits from both AsyncWrap
and ThreadPoolWork, we add a `CompressionStream::env()` getter that
delegates to `AsyncWrap::env()`.

Refs: #31554 (comment)
@nodejs-github-bot

This comment has been minimized.

@bnoordhuis bnoordhuis mentioned this pull request Jan 31, 2020
2 of 2 tasks complete
@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Jan 31, 2020

Can I ask that you add @ConorDavenport as a co-author on the first commit here

@bnoordhuis

This comment has been minimized.

Copy link
Member Author

bnoordhuis commented Jan 31, 2020

I wrote it from scratch, I didn't copy anything from the other PR.

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Jan 31, 2020

I wrote it from scratch, I didn't copy anything from the other PR.

Yet it duplicates much of that and it's unlikely you would have opened this if not for the other. Is there some kind of harm in listing a co-author that I'm not aware of? Doing so does seem friendlier to a new contributor.

@bnoordhuis

This comment has been minimized.

Copy link
Member Author

bnoordhuis commented Jan 31, 2020

Is there some kind of harm in listing a co-author that I'm not aware of?

I only do that when I take someone's existing patch and modify it.

Copy link
Member

jasnell left a comment

Prefer that a fixed up version of #31554 land instead to cover the CryptoJob fixes. The stream_env() changes in this PR are good.

@bnoordhuis

This comment has been minimized.

Copy link
Member Author

bnoordhuis commented Feb 2, 2020

You should motivate why. If it's just because you're not getting your way, well, that's an abuse of the 'Request changes' button, isn't it?

@bnoordhuis

This comment has been minimized.

Copy link
Member Author

bnoordhuis commented Feb 3, 2020

I'm labeling this tsc-agenda. What @jasnell did is a clear violation of process and an abuse of privileges. That's unacceptable from any collaborator but especially a TSC member.

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Feb 3, 2020

I'm labeling this tsc-agenda. What @jasnell did is a clear violation of process and an abuse of privileges. That's unacceptable from any collaborator but especially a TSC member.

The other PR was a) opened first, b) had adequate sign-off and review with no -1's, c) passed CI, d) was updated to address your concern, e) was well passed the 48 hour minimal period to land, f) had zero reasons not to be landed.

@bnoordhuis

This comment has been minimized.

Copy link
Member Author

bnoordhuis commented Feb 3, 2020

You blocked this PR from getting merged without giving any motivation, even after I requested it. Instead you quietly landed a competing PR that contains changes of your own. That is not appropriate behavior for a TSC member.

@cjihrig

This comment has been minimized.

Copy link
Contributor

cjihrig commented Feb 3, 2020

@bnoordhuis by adding this to the TSC agenda, what outcome are you advocating for? My assumption would be a revert of #31554, but I don't want to guess incorrectly. Alternatively, you could probably join the meeting yourself to discuss it.

FWIW, I do believe that blocking this PR was inappropriate, and #31588 (comment) is an accurate description of what happened.

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Feb 3, 2020

Feel free to revert #31554 if you'd like. I'm not going to engage. We've had many instances of -1's on PRs in favor of competing PRs.

@jasnell jasnell dismissed their stale review Feb 3, 2020

Dismissing

@bnoordhuis

This comment has been minimized.

Copy link
Member Author

bnoordhuis commented Feb 3, 2020

@cjihrig I'm okay with leaving in #31554. I don't want to tell the TSC what to do but I do feel it's important that misconduct from one of its members is discussed.

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Feb 3, 2020

My apologies for any perceived misconduct. In my -1 on this I did give a justification in that I preferred the changes made in the other PR. This one makes much more extensive changes that may be nice-to-haves but are, in my opinion, better to be split into multiple PRs (specifically, I'd rather see the stream_env commit in a separate PR).

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Feb 10, 2020

There hasn’t been any further discussion since last week’s TSC meeting, so it doesn’t seem to make sense to have it on this week’s agenda. I’ll remove the label.

@addaleax addaleax removed the tsc-agenda label Feb 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.