doc: clarify assigning issues to the TSC by fhinkel · Pull Request #22759 · nodejs/node · GitHub

Using the right hand side to assign @nodejs/tsc or to request reviews from @nodejs/tsc breaks all kinds of workflows and filter rules. Please use the appropriate tsc-review label or mention @nodejs/tsc in a comment instead.

@fhinkel build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/830/pipeline

Hmm... breaks workflows how? I'm not sure I agree with a restriction not to use the GitHub UI for requesting TSC review.

There's no way to filter requests to a specific person or to teams. Please don't use it.

As a non-contributing observer, I agree with @fhinkel

Pull requests -> Reviews requests lists PRs where my reviews are requested. Right now this list is useless because of all the TSC items. Doesn't matter if I reviewed it or not. As long as TSC is assigned, it's rendering my to-do list useless.

@fhinkel I use that feature to request reviews from the TSC for semver-major changes or in very rare cases, PRs that are controversial. Would it therefore actually not be good to see that PR in your list?

In my experience, the review label was completely ignored so far.

I just put one PR back on the TSC agenda as well, since not enough members reacted to your request to provide feedback to two competing PRs.

Yes, it's very annoying to get those requests. Instead of making me anything review faster, I don't review anything at all because it's too much spam and end up missing requests targeted directly at me.

But how is something spam that only the TSC can may do?

Nit: mentioning -> mention

It's spam because it goes to 19 people independent of whether they've already reviewed it. I have a day job. I want to look at TSC issues after hours and not have them in my list. Partially my fault for not using separate accounts. But when using the same account theres no way to separate them.

If you use the label or mention, I can grab for the label or search my inbox for TSC reviews.

And it's not like TSC members respond to the group review request, so you might as well not do it and save me a big headache.

LGTM. I still wonder how we could improve the TSC reviewing part in general.

Pinging the right individual people directly usually works better. Either via mention or Twitter/email. There's so much noise on GitHub, it's really hard to stay on top.

Unaware about the internal workflow complexity, but convincing to hear from the person who manages that. In addition, it brings the best use of the label - if an action with same purpose is carried out through multiple means, the rule gets vague. This brings clarity to those.

@fhinkel FWIW this is how I fixed this issue on my side: Pull Requests -> Review requests -> Add org:org_name to the query -> Bookmark URL

Edit: you can also keep everything but node with -org:nodejs

Nit: extra backtick at the line end.

Rather than making this a hard rule without documenting the reasons for it, consider rewording as a suggestion with the reasons why one might want to avoid it. Also, consider, that the same likely applies to requesting review from any team, not just the @nodejs/tsc.

The following is too wordy given that it's 6:30am on a Saturday and I haven't had enough coffee but this should illustrate the point:

Use of the GitHub UI to assign an issue to, or request a review
from the `@nodejs/tsc` (or any team) is not recommended because,
at the current time, doing so causes unnecessary notifications to
be sent to team members who may already have provided feedback or
reviews for the issue or PR. It is better to selectively request
reviews on an individual basis.

More Friendly-Advice-To-Make-Co-Contributors-Lives-Easier, less Yet-Another-Rule-One-Has-To-Follow.

Would it still be fine to use it once in the beginning? That way it would show up for the whole TSC. Just assigning it a second time would be discouraged after the first TSC member looked at it?

What's the hoped-for result that wouldn't be achieved with an @-mention?

I do think that getting TSC reviews is a bit of a problem right now. I think there are some culture/people changes that need to happen more than process changes to fix that, though.

LGTM

CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/1112/

PR-URL: nodejs#22759 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

Landed in af522c1

PR-URL: #22759 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Sep 7, 2018

fhinkel requested a review from ofrobots September 7, 2018 23:50

Trott reviewed Sep 8, 2018

View reviewed changes

fhinkel force-pushed the master branch from aa494f0 to 2b858b8 Compare September 8, 2018 00:58

BridgeAR approved these changes Sep 8, 2018

View reviewed changes

Trott approved these changes Sep 8, 2018

View reviewed changes

gireeshpunathil approved these changes Sep 8, 2018

View reviewed changes

apapirovski approved these changes Sep 8, 2018

View reviewed changes

trivikr approved these changes Sep 8, 2018

View reviewed changes

targos approved these changes Sep 8, 2018

View reviewed changes

vsemozhetbyt reviewed Sep 8, 2018

View reviewed changes

lpinca approved these changes Sep 8, 2018

View reviewed changes

addaleax approved these changes Sep 8, 2018

View reviewed changes

doc: clarify assigning issues to the TSC

73064ba

fhinkel force-pushed the master branch from 2b858b8 to 73064ba Compare September 9, 2018 00:55

cjihrig approved these changes Sep 9, 2018

View reviewed changes

BridgeAR reviewed Sep 10, 2018

View reviewed changes

mhdawson approved these changes Sep 27, 2018

View reviewed changes

thefourtheye approved these changes Sep 27, 2018

View reviewed changes

Trott added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Oct 3, 2018

Trott closed this Oct 3, 2018

targos mentioned this pull request Oct 7, 2018

Release proposal: v10.12.0 #23313

Merged

cjihrig mentioned this pull request Jan 6, 2025

doc: allow request for TSC reviews via the GitHub UI #56493

Merged

+              Assign the `tsc-review` label or @-mention the
+              `@nodejs/tsc` GitHub team if you want to elevate an issue to the [TSC][].
+              Do not use the GitHub UI on the right hand side to assign to
+              `@nodejs/tsc` or request a review from `@nodejs/tsc`.

doc: clarify assigning issues to the TSC by fhinkel · Pull Request #22759 · nodejs/node · GitHub

Uh oh!

Uh oh!

doc: clarify assigning issues to the TSC - #22759

doc: clarify assigning issues to the TSC#22759fhinkel wants to merge 1 commit intonodejs:masternodejs/node:masterfrom fhinkel:masterfhinkel/node:masterCopy head branch name to clipboard

Conversation

fhinkel commented Sep 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 7, 2018

Uh oh!

jasnell commented Sep 7, 2018

Uh oh!

fhinkel commented Sep 7, 2018

Uh oh!

kadeatfox commented Sep 7, 2018

Uh oh!

fhinkel commented Sep 7, 2018

Uh oh!

BridgeAR commented Sep 8, 2018

Uh oh!

BridgeAR commented Sep 8, 2018

Uh oh!

BridgeAR commented Sep 8, 2018

Uh oh!

fhinkel commented Sep 8, 2018

Uh oh!

BridgeAR commented Sep 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott Sep 8, 2018

Choose a reason for hiding this comment

Uh oh!

fhinkel commented Sep 8, 2018

Uh oh!

fhinkel commented Sep 8, 2018

Uh oh!

fhinkel commented Sep 8, 2018

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

fhinkel commented Sep 8, 2018

Uh oh!

gireeshpunathil commented Sep 8, 2018

Uh oh!

targos commented Sep 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vsemozhetbyt Sep 8, 2018

Choose a reason for hiding this comment

Uh oh!

jasnell Sep 8, 2018

Choose a reason for hiding this comment

Uh oh!

BridgeAR Sep 10, 2018

Choose a reason for hiding this comment

Uh oh!

Trott Oct 3, 2018

Choose a reason for hiding this comment

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Oct 3, 2018

Uh oh!

Trott commented Oct 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

16 participants

doc: clarify assigning issues to the TSC#22759
fhinkel wants to merge 1 commit into
nodejs:masterfrom
fhinkel:master

fhinkel commented Sep 7, 2018 •
edited
Loading

BridgeAR commented Sep 8, 2018 •
edited
Loading

targos commented Sep 8, 2018 •
edited
Loading