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

meta: Notify Zulip for rustdoc nominated issues #82689

Merged
merged 1 commit into from
Mar 3, 2021

Conversation

camelid
Copy link
Member

@camelid camelid commented Mar 2, 2021

The rustdoc team does not currently use the I-nominated label, unlike
the libs and compiler teams (and maybe others). One reason for this is
that the other teams discuss their nominated issues in meetings, while
rustdoc is an async-only team.

However, it might be helpful to start using the I-nominated label for
rustdoc. The team currently uses a cc @rust-lang/rustdoc ping as the
equivalent, but it's easier to track issues when they use I-nominated.
Also we'd be more consistent with the other teams' procedures.

Since rustdoc doesn't have meetings, I propose we instead use the
triagebot notify Zulip functionality to create a topic in #rustdoc on
Zulip and ping the team. So it would look a bit like the procedure for
WG-prioritization when an issue acquires the I-prioritize label.

cc @rust-lang/rustdoc for approval of the change
r? @Mark-Simulacrum to make sure I configured triagebot correctly

@camelid camelid added A-meta Area: Issues about the rust-lang/rust repository. I-needs-decision Issue: In need of a decision. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Mar 2, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 2, 2021
triagebot.toml Outdated Show resolved Hide resolved
triagebot.toml Outdated Show resolved Hide resolved
triagebot.toml Outdated Show resolved Hide resolved
@jyn514
Copy link
Member

jyn514 commented Mar 2, 2021

This was a good idea :)

@Manishearth
Copy link
Member

@camelid feel free to r=me once you've figured out what to do about your comments on the PR.

The rustdoc team does not currently use the `I-nominated` label, unlike
the libs and compiler teams (and maybe others). One reason for this is
that the other teams discuss their nominated issues in meetings, while
rustdoc is an async-only team.

However, it might be helpful to start using the `I-nominated` label for
rustdoc. The team currently uses a `cc @rust-lang/rustdoc` ping as the
equivalent, but it's easier to track issues when they use `I-nominated`.
Also we'd be more consistent with the other teams' procedures.

Since rustdoc doesn't have meetings, I propose we instead use the
triagebot notify Zulip functionality to create a topic in `#rustdoc` on
Zulip and ping the team. So it would look a bit like the procedure for
WG-prioritization when an issue acquires the `I-prioritize` label.
@camelid
Copy link
Member Author

camelid commented Mar 2, 2021

(Squashed.)

Comment on lines +105 to +112
[notify-zulip."T-rustdoc"]
required_labels = ["I-nominated"]
zulip_stream = 266220 # #rustdoc
topic = "nominated: #{number}"
message_on_add = """\
@*T-rustdoc* issue #{number} "{title}" has been nominated for `T-rustdoc` discussion.
"""
message_on_remove = "Issue #{number}'s nomination request has been removed."
Copy link
Member Author

Choose a reason for hiding this comment

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

The one issue with this setup is that we will only be notified if T-rustdoc is removed—nothing will happen if I-nominated is removed. It might be worth getting rid of the message_on_remove since it will be kind of confusing.

Copy link
Member

Choose a reason for hiding this comment

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

Does an array work here? cc @Mark-Simulacrum can we do [[notify-zulip."I-nominated"]] here?

Copy link
Member

Choose a reason for hiding this comment

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

No, I don't think so, but I also don't really know what you mean by that. That said, it seems quite reasonable for triagebot to detect the removal of a required label and treat that just like removal of T-rustdoc; I think the reasonable treatment is that the whole set of labels must be present and removal of any leads to a notification of such. Happy to accept a PR to that effect.

Copy link
Member Author

@camelid camelid Mar 2, 2021

Choose a reason for hiding this comment

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

That said, it seems quite reasonable for triagebot to detect the removal of a required label and treat that just like removal of T-rustdoc; I think the reasonable treatment is that the whole set of labels must be present and removal of any leads to a notification of such. Happy to accept a PR to that effect.

That might be a natural extension of the work in rust-lang/triagebot#1078Unlabeled would be treated the same as Closed. Probably not worth blocking this PR on that one though.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I don't think so, but I also don't really know what you mean by that.

I think he means that we would be notified as soon as both I-nominated and T-rustdoc were present, and we would be notified again as soon as one of them was removed. Treating the removal of a required label the same as the removal of the main label would have the same effect, but gives the labels an asymmetric relationship (T-rustdoc is considered the "main" label by triagebot).

@camelid
Copy link
Member Author

camelid commented Mar 2, 2021

I think I'll wait until one or two more people approve, and for feedback on #82689 (comment), before I merge.

@Mark-Simulacrum
Copy link
Member

Broadly looks good to me, but these things are finicky, so I recommend merging and trying it out.

@camelid
Copy link
Member Author

camelid commented Mar 2, 2021

@bors r=Manishearth

@bors
Copy link
Contributor

bors commented Mar 2, 2021

📌 Commit 59ee872 has been approved by Manishearth

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Mar 2, 2021
@camelid camelid removed the I-needs-decision Issue: In need of a decision. label Mar 2, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 3, 2021
Rollup of 10 pull requests

Successful merges:

 - rust-lang#81223 ([rustdoc] Generate redirect map file)
 - rust-lang#82439 (BTree: fix untrue safety)
 - rust-lang#82469 (Use a crate to produce rustdoc tree comparisons instead of the `diff` command)
 - rust-lang#82589 (unix: Non-mutable bufs in send_vectored_with_ancillary_to)
 - rust-lang#82689 (meta: Notify Zulip for rustdoc nominated issues)
 - rust-lang#82695 (Revert non-power-of-two vector restriction)
 - rust-lang#82706 (use outer_expn_data() instead of outer_expn().expn_data())
 - rust-lang#82710 (FloatToInit: Replacing round_unchecked_to --> to_int_unchecked)
 - rust-lang#82712 (Remove unnecessary conditional `cfg(target_os)` for `redox` and `vxworks`)
 - rust-lang#82713 (Update cargo)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9b58b8b into rust-lang:master Mar 3, 2021
@rustbot rustbot added this to the 1.52.0 milestone Mar 3, 2021
@camelid camelid deleted the rustdoc-nominated branch March 3, 2021 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-meta Area: Issues about the rust-lang/rust repository. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants