src: remove duplicate env field from CryptoJob#31588
src: remove duplicate env field from CryptoJob#31588bnoordhuis wants to merge 2 commits intonodejs:masterfrom
Conversation
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: nodejs#31554 (comment)
Rename `StreamBase::stream_env()` to `StreamBase::env()` and work around the diamond problem by adding `env()` getters to several classes. Refs: nodejs#31554 (comment)
|
Can I ask that you add @conordavenport as a co-author on the first commit here |
|
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. |
I only do that when I take someone's existing patch and modify it. |
|
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? |
|
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. |
|
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. |
|
@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. |
|
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. |
|
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). |
|
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. |
8ae28ff to
2935f72
Compare
|
Closing because this has stalled. Can reopen if there is a desire to revisit |
Alternative take on #31554 that tightens up the use of
env()getters.First:
And then: