Stream: t-compiler/wg-prioritization

Topic: triagebot pendings


Santiago Pastorino (May 14 2020 at 15:33, on Zulip):

yesterday I was commenting @LeSeulArtichaut that would be great to fire prioritization when we add regression-from-stable-to-beta and regression-from-stable-to-nightly labels to issues

Santiago Pastorino (May 14 2020 at 15:34, on Zulip):

we ended also talking that prioritization was fired twice when someone runs the command because it also add the label and that re-fires the thing

Santiago Pastorino (May 14 2020 at 15:34, on Zulip):

@simulacrum what code is executed when a github label is added?

simulacrum (May 14 2020 at 15:34, on Zulip):

not sure what you're asking

simulacrum (May 14 2020 at 15:34, on Zulip):

like, a lot of code? :)

Santiago Pastorino (May 14 2020 at 15:35, on Zulip):

I'm not sure how things are connected here, does Github fires a post to us somewhere?

simulacrum (May 14 2020 at 15:35, on Zulip):

yeah

Santiago Pastorino (May 14 2020 at 15:35, on Zulip):

where does that land?

simulacrum (May 14 2020 at 15:35, on Zulip):

triagebot will get a webhook post

Santiago Pastorino (May 14 2020 at 15:35, on Zulip):

in triagebot?

Santiago Pastorino (May 14 2020 at 15:35, on Zulip):

ok

LeSeulArtichaut (May 14 2020 at 15:35, on Zulip):

I looked at the code earlier

LeSeulArtichaut (May 14 2020 at 15:35, on Zulip):

We fire the prioritize command when the I-prioritize label is added

LeSeulArtichaut (May 14 2020 at 15:35, on Zulip):

Which means, as the prioritize command adds the label, the command is fired twice

Santiago Pastorino (May 14 2020 at 15:36, on Zulip):

yep

LeSeulArtichaut (May 14 2020 at 15:36, on Zulip):

One solution is to not create the Zulip thread/message when @rustbot prioritize is used, and delay it when the label is added

simulacrum (May 14 2020 at 15:37, on Zulip):

we should be able to ignore the label if it's added by the bot

LeSeulArtichaut (May 14 2020 at 15:38, on Zulip):

Oh right, we recieve it along with the webhook event

Santiago Pastorino (May 14 2020 at 15:38, on Zulip):

can't we make the prioritization process start only when the label is added?

Santiago Pastorino (May 14 2020 at 15:38, on Zulip):

and make the command just add that label

LeSeulArtichaut (May 14 2020 at 15:38, on Zulip):

The problem is

LeSeulArtichaut (May 14 2020 at 15:39, on Zulip):

If we ignore when the bot adds the label, then we won't create the Zulip post when people use the @rustbot modify labels command

simulacrum (May 14 2020 at 15:39, on Zulip):

yeah seems fine to just trigger zulip on the label and not the command

LeSeulArtichaut (May 14 2020 at 15:39, on Zulip):

Santiago Pastorino said:

can't we make the prioritization process start only when the label is added?

That's what I proposed

LeSeulArtichaut (May 14 2020 at 15:40, on Zulip):

Alright I can do that then

LeSeulArtichaut (May 14 2020 at 15:40, on Zulip):

Quickly

Santiago Pastorino (May 14 2020 at 15:40, on Zulip):

:+1:

Santiago Pastorino (May 14 2020 at 15:41, on Zulip):

I was wondering also ...

Santiago Pastorino (May 14 2020 at 15:41, on Zulip):
if let Some(Token::Word("prioritize")) = input.peek_token()? {
Santiago Pastorino (May 14 2020 at 15:41, on Zulip):

are we checking that the label contains prioritize?

LeSeulArtichaut (May 14 2020 at 15:41, on Zulip):

That's for the command I guess

simulacrum (May 14 2020 at 15:41, on Zulip):

that's the parser, not the label

Santiago Pastorino (May 14 2020 at 15:41, on Zulip):

I just naively run rg "I-prioritize" and didn't find anything

LeSeulArtichaut (May 14 2020 at 15:41, on Zulip):

The @rustbot prioritize command, I mean

Santiago Pastorino (May 14 2020 at 15:41, on Zulip):

ahh right

LeSeulArtichaut (May 14 2020 at 15:42, on Zulip):

The label is stored in triagebot.toml file

simulacrum (May 14 2020 at 15:42, on Zulip):

the label is configured in triagebot.toml in rust-lang/rust

LeSeulArtichaut (May 14 2020 at 15:42, on Zulip):

On the repo

Santiago Pastorino (May 14 2020 at 15:42, on Zulip):

ohh I see

LeSeulArtichaut (May 14 2020 at 15:42, on Zulip):

So if we want to prioritize when certain labels are added we should do that in the triagebot.toml too, right?

Santiago Pastorino (May 14 2020 at 15:43, on Zulip):

I guess we may want something like

LeSeulArtichaut (May 14 2020 at 15:43, on Zulip):

We could have a prioritize-on field for example

Santiago Pastorino (May 14 2020 at 15:43, on Zulip):
[prioritize]
label = ["I-prioritize", "regression-from-stable-to-beta", "regression-from-stable-to-nightly"]
zulip_stream = 227806
Santiago Pastorino (May 14 2020 at 15:43, on Zulip):

maybe?

Santiago Pastorino (May 14 2020 at 15:44, on Zulip):

instead of what we have which is ...

LeSeulArtichaut (May 14 2020 at 15:44, on Zulip):

The label field here is the label we assign when we use the prioritize command

Santiago Pastorino (May 14 2020 at 15:44, on Zulip):

ahh :)

LeSeulArtichaut (May 14 2020 at 15:44, on Zulip):

And also the label the bot reacts to when added to create the Zulip topic

LeSeulArtichaut (May 14 2020 at 15:44, on Zulip):

We can have a separate list of course

Santiago Pastorino (May 14 2020 at 15:45, on Zulip):

yeah then what you've said, prioritize-on

LeSeulArtichaut (May 14 2020 at 15:45, on Zulip):

What would you prefer? Adding I-prioritize when regression are found?

LeSeulArtichaut (May 14 2020 at 15:45, on Zulip):

Or sending a message without adding I-prioritize?

Santiago Pastorino (May 14 2020 at 15:46, on Zulip):

it's the same

Santiago Pastorino (May 14 2020 at 15:46, on Zulip):

maybe better and easier to add I-prioritize

LeSeulArtichaut (May 14 2020 at 15:46, on Zulip):

Personally I prefer the first solution, because it shows that we have a discussion running

LeSeulArtichaut (May 14 2020 at 15:46, on Zulip):

And when we're done then we can remove it

Santiago Pastorino (May 14 2020 at 15:46, on Zulip):

yeah, agree, let's add I-prioritize

Santiago Pastorino (May 14 2020 at 15:46, on Zulip):

definitely

Santiago Pastorino (May 14 2020 at 15:46, on Zulip):

are you doing this or should I?

LeSeulArtichaut (May 14 2020 at 15:47, on Zulip):

I don't know

LeSeulArtichaut (May 14 2020 at 15:47, on Zulip):

I'll be fixing the bug with the command first I think

Santiago Pastorino (May 14 2020 at 15:47, on Zulip):

:+1:

LeSeulArtichaut (May 14 2020 at 15:47, on Zulip):

Because then we'll have a clean code for implementing that new feature

Santiago Pastorino (May 14 2020 at 15:47, on Zulip):

ok, let me know when you finish

LeSeulArtichaut (May 14 2020 at 15:48, on Zulip):

I'll send a link to the PR as soon as I'm done

Santiago Pastorino (May 14 2020 at 15:48, on Zulip):

:+1:, great, thanks

LeSeulArtichaut (May 14 2020 at 15:48, on Zulip):

I'll also ping simulacrum :innocent:

Santiago Pastorino (May 14 2020 at 15:50, on Zulip):

:)

Santiago Pastorino (May 14 2020 at 15:54, on Zulip):
[santiago@galago triagebot (master)]$ git diff
diff --git a/src/handlers/relabel.rs b/src/handlers/relabel.rs
index 9a889a5..e6c8a56 100644
--- a/src/handlers/relabel.rs
+++ b/src/handlers/relabel.rs
@@ -105,6 +105,15 @@ async fn handle_input(
                     issue_labels.push(github::Label {
                         name: label.to_string(),
                     });
+                    if label.as_str() == "regression-from-stable-to-beta"
+                        || label.as_str() == "regression-from-stable-to-nightly"
+                    {
+                        if !issue_labels.iter().any(|l| l.name == "I-prioritize") {
+                            issue_labels.push(github::Label {
+                                name: "I-prioritize".to_string(),
+                            });
+                        }
+                    }
                 }
             }
             LabelDelta::Remove(label) => {
Santiago Pastorino (May 14 2020 at 15:54, on Zulip):

done

Santiago Pastorino (May 14 2020 at 15:54, on Zulip):

:joy:

LeSeulArtichaut (May 14 2020 at 15:54, on Zulip):

:D

simulacrum (May 14 2020 at 15:55, on Zulip):

No, no, that's not right

simulacrum (May 14 2020 at 15:55, on Zulip):

we don't want that as part of relabeling

simulacrum (May 14 2020 at 15:55, on Zulip):

because you want it to happen when someone adds the label through github ui

simulacrum (May 14 2020 at 15:55, on Zulip):

so you'll want to do it in the prioritization handler

LeSeulArtichaut (May 14 2020 at 15:56, on Zulip):

Also it's the first time I use rust-analyser outside of rustc

LeSeulArtichaut (May 14 2020 at 15:56, on Zulip):

It's amazing :eyes:

Santiago Pastorino (May 14 2020 at 15:57, on Zulip):

simulacrum said:

because you want it to happen when someone adds the label through github ui

maybe I got wrong what relabel is

Santiago Pastorino (May 14 2020 at 15:57, on Zulip):

but yeah, we want to do it once any of those labels are added

Santiago Pastorino (May 14 2020 at 15:57, on Zulip):

not only through github ui

LeSeulArtichaut (May 14 2020 at 15:57, on Zulip):

Look at src/handlers/prioritize.rs line 34 xD

LeSeulArtichaut (May 14 2020 at 15:57, on Zulip):

Currently we have:

                    if e.label.as_ref().expect("label").name == config.label {
                        // We need to take the exact same action in this case.
                        return Ok(Some(Prioritize::Start));
                    }
Santiago Pastorino (May 14 2020 at 15:58, on Zulip):

:+1:

Santiago Pastorino (May 14 2020 at 15:58, on Zulip):

I was kind of kidding and playing a bit

Santiago Pastorino (May 14 2020 at 15:58, on Zulip):

but maybe I should just implement this :)

LeSeulArtichaut (May 14 2020 at 15:58, on Zulip):

So IIUC we fire a prioritize command if the label matches the label in the config

LeSeulArtichaut (May 14 2020 at 15:58, on Zulip):

Santiago Pastorino said:

I was kind of kidding and playing a bit

Don't worry I got it :laughing:

Santiago Pastorino (May 14 2020 at 15:59, on Zulip):

anyway kidding about hard coding the things

Santiago Pastorino (May 14 2020 at 15:59, on Zulip):

but now that I see prioritize handler it's more clear

LeSeulArtichaut (May 14 2020 at 15:59, on Zulip):

:+1:

Santiago Pastorino (May 14 2020 at 16:05, on Zulip):

#72200

LeSeulArtichaut (May 14 2020 at 16:05, on Zulip):

Damn you're quick x)

LeSeulArtichaut (May 14 2020 at 16:06, on Zulip):

Not sure we want to include I-prioritize in the list though

LeSeulArtichaut (May 14 2020 at 16:06, on Zulip):

Because the behaviour will be to add I-prioritize whenever those are added

LeSeulArtichaut (May 14 2020 at 16:06, on Zulip):

@Santiago Pastorino

Santiago Pastorino (May 14 2020 at 16:07, on Zulip):

you're right

Santiago Pastorino (May 14 2020 at 16:07, on Zulip):

maybe with this idea the name is not great

Santiago Pastorino (May 14 2020 at 16:07, on Zulip):

also_prioritize_on

Santiago Pastorino (May 14 2020 at 16:07, on Zulip):

well it's fine as is I guess :)

LeSeulArtichaut (May 14 2020 at 16:07, on Zulip):

I also think so

LeSeulArtichaut (May 14 2020 at 16:08, on Zulip):

When I threw it I was about to say please send help for a better name

Santiago Pastorino (May 14 2020 at 16:08, on Zulip):

but I think it's ok

Santiago Pastorino (May 14 2020 at 16:09, on Zulip):

because clearly I-prioritize is a request for prioritization

Santiago Pastorino (May 14 2020 at 16:09, on Zulip):

fixed the PR

LeSeulArtichaut (May 14 2020 at 16:09, on Zulip):

:+1:

Santiago Pastorino (May 14 2020 at 16:09, on Zulip):

polishing a bit the other part

Santiago Pastorino (May 14 2020 at 16:12, on Zulip):

I think it's just

Santiago Pastorino (May 14 2020 at 16:12, on Zulip):
diff --git a/src/config.rs b/src/config.rs
index 0c6f5f2..550dff0 100644
--- a/src/config.rs
+++ b/src/config.rs
@@ -78,6 +78,7 @@ pub(crate) struct RelabelConfig {
 #[derive(PartialEq, Eq, Debug, serde::Deserialize)]
 pub(crate) struct PrioritizeConfig {
     pub(crate) label: String,
+    pub(crate) prioritize_on: Vec<String>,
     pub(crate) zulip_stream: u64,
 }

diff --git a/src/handlers/prioritize.rs b/src/handlers/prioritize.rs
index f965422..21a735b 100644
--- a/src/handlers/prioritize.rs
+++ b/src/handlers/prioritize.rs
@@ -34,7 +34,11 @@ impl Handler for PrioritizeHandler {
         if let Event::Issue(e) = event {
             if e.action == github::IssuesAction::Labeled {
                 if let Some(config) = config {
-                    if e.label.as_ref().expect("label").name == config.label {
+                    if config
+                        .prioritize_on
+                        .iter()
+                        .any(|l| l == &e.label.as_ref().expect("label").name)
+                    {
                         // We need to take the exact same action in this case.
                         return Ok(Some(Prioritize::Start));
                     }
Santiago Pastorino (May 14 2020 at 16:12, on Zulip):

we don't want to do anything when the label is removed

Santiago Pastorino (May 14 2020 at 16:13, on Zulip):

to remove the request from prioritization is removing I-prioritize, nothing should happen if regression related labels are removed

Santiago Pastorino (May 14 2020 at 16:15, on Zulip):

opened too triagebot#542

LeSeulArtichaut (May 14 2020 at 16:20, on Zulip):

Hm, probably stable-to-stable too?

Do we want to prioritize on stable regressions

LeSeulArtichaut (May 14 2020 at 16:20, on Zulip):

I also think we should

Santiago Pastorino (May 14 2020 at 16:20, on Zulip):

commented, I think we should yeah

Santiago Pastorino (May 14 2020 at 16:21, on Zulip):

there are 22 unprioritized regressions right now

LeSeulArtichaut (May 14 2020 at 16:21, on Zulip):

Oh also @Santiago Pastorino, should we prioritize even if the issue is already prioritized?

Santiago Pastorino (May 14 2020 at 16:22, on Zulip):

you're right, we should not request prioritization if it's P-* already

LeSeulArtichaut (May 14 2020 at 16:22, on Zulip):

So maybe add (another) field in config

Santiago Pastorino (May 14 2020 at 16:22, on Zulip):

hehe :+1:

LeSeulArtichaut (May 14 2020 at 16:22, on Zulip):

To describe what a priority is

LeSeulArtichaut (May 14 2020 at 16:22, on Zulip):

I guess we can reuse code from the relabel command

LeSeulArtichaut (May 14 2020 at 16:23, on Zulip):

Which accepted those blob patterns (not sure what there're called)

Santiago Pastorino (May 14 2020 at 16:23, on Zulip):

LeSeulArtichaut said:

I guess we can reuse code from the relabel command

what do you mean?

LeSeulArtichaut (May 14 2020 at 16:24, on Zulip):

In the configuration for the relabel command, triagebot accepts those patterns

Santiago Pastorino (May 14 2020 at 16:24, on Zulip):

also added a new attribute to #72200

LeSeulArtichaut (May 14 2020 at 16:24, on Zulip):

So you should be able to reuse code from there

LeSeulArtichaut (May 14 2020 at 16:25, on Zulip):

Having P-* in the config would be more convenient

LeSeulArtichaut (May 14 2020 at 16:25, on Zulip):

But that's a detail anyway :P

Santiago Pastorino (May 14 2020 at 16:25, on Zulip):

so yeah, I thought about that but I was worried a bit that we may have P-something-not-priority

Santiago Pastorino (May 14 2020 at 16:25, on Zulip):

at some point I thought we were going to call I-prioritize P-prioritize

Santiago Pastorino (May 14 2020 at 16:26, on Zulip):

but yeah, what I'm saying probably doesn't make sense

Santiago Pastorino (May 14 2020 at 16:26, on Zulip):

:)

Santiago Pastorino (May 14 2020 at 16:26, on Zulip):

will do a pattern

LeSeulArtichaut (May 14 2020 at 16:26, on Zulip):

Right, we might no have the P- prefix monopoly forever :D

Santiago Pastorino (May 14 2020 at 16:27, on Zulip):

pushed with the pattern

Santiago Pastorino (May 14 2020 at 16:27, on Zulip):

priorities_pattern = "P-*"

Santiago Pastorino (May 14 2020 at 16:27, on Zulip):

will fix triagebot code after lunch

LeSeulArtichaut (May 14 2020 at 16:29, on Zulip):

Fun fact: if you type @spastorino on GitHub the UI suggests you @rust-lang/cargo-taggers :big_smile:

LeSeulArtichaut (May 14 2020 at 16:35, on Zulip):

Opened triagebot#543

LeSeulArtichaut (May 14 2020 at 16:36, on Zulip):

@simulacrum Do you have time to review this one? It should hopefully be pretty quick :big_smile:

simulacrum (May 14 2020 at 16:55, on Zulip):

yeah I can take a look

LeSeulArtichaut (May 14 2020 at 16:55, on Zulip):

Thanks :+1:

Santiago Pastorino (May 14 2020 at 17:04, on Zulip):

LeSeulArtichaut said:

Fun fact: if you type @spastorino on GitHub the UI suggests you @rust-lang/cargo-taggers :big_smile:

WAT?

Santiago Pastorino (May 14 2020 at 17:04, on Zulip):

hahahaha

LeSeulArtichaut (May 14 2020 at 17:04, on Zulip):

:shrug:

LeSeulArtichaut (May 14 2020 at 17:04, on Zulip):

Is that a coincidence? I don't think so... :eyes:

LeSeulArtichaut (May 14 2020 at 17:10, on Zulip):

triagebot#543 just landed (thanks again @simulacrum!)
@Santiago Pastorino I think you'll want to rebase your PR onto master now. To make triagebot add the I-prioritize label, you'll now want to return Prioritize::Label instead of Prioritize::Start.

Santiago Pastorino (May 14 2020 at 17:19, on Zulip):

yeah, just got back

Santiago Pastorino (May 14 2020 at 17:19, on Zulip):

done and force pushed with the proper fixes

Santiago Pastorino (May 14 2020 at 17:20, on Zulip):

ahh let me mark the fields with serde(default)

Santiago Pastorino (May 14 2020 at 17:21, on Zulip):

ok, done

Santiago Pastorino (May 14 2020 at 17:22, on Zulip):

#72200 and triagebot#542 should be both ready

Santiago Pastorino (May 14 2020 at 17:42, on Zulip):

hold on what I did is wrong

Santiago Pastorino (May 14 2020 at 17:42, on Zulip):

wasn't paying attention

LeSeulArtichaut (May 14 2020 at 17:42, on Zulip):

:innocent:

LeSeulArtichaut (May 14 2020 at 17:45, on Zulip):

By the way if someone has to prioritize an issue, I would be grateful if someone tests using @rustbot prioritize

Santiago Pastorino (May 14 2020 at 17:50, on Zulip):

:+1:

Santiago Pastorino (May 14 2020 at 17:50, on Zulip):

pushed

Santiago Pastorino (May 14 2020 at 17:50, on Zulip):

now yes

Santiago Pastorino (May 14 2020 at 17:50, on Zulip):

"should" be ready

Santiago Pastorino (May 14 2020 at 17:51, on Zulip):

@LeSeulArtichaut about https://github.com/rust-lang/triagebot/pull/542/files#r425313569, I like your name better but do you think would be better using a Vec<String> here?

Santiago Pastorino (May 14 2020 at 17:52, on Zulip):

I guess we won't have really complex patterns to find labels

LeSeulArtichaut (May 14 2020 at 17:52, on Zulip):

I think it would be consistent with the relabel command

LeSeulArtichaut (May 14 2020 at 17:52, on Zulip):

That's a nitpick, really

Santiago Pastorino (May 14 2020 at 17:52, on Zulip):

yeah but I'm not even reusing the code because it's just using glob in my case

Santiago Pastorino (May 14 2020 at 17:54, on Zulip):

renamed it

LeSeulArtichaut (May 14 2020 at 17:54, on Zulip):

However I still think the logic is wrong...

LeSeulArtichaut (May 14 2020 at 17:55, on Zulip):

We still need to have this part of the code:

if e.label.as_ref().expect("label").name == config.label {
    // We need to take the exact same action in this case.
     return Ok(Some(Prioritize::Start));
}

Because I rely on it to start the Zulip thread

LeSeulArtichaut (May 14 2020 at 17:56, on Zulip):

I think we want to keep it, and append your code

LeSeulArtichaut (May 14 2020 at 18:00, on Zulip):

Also return Prioritize::Label in this case

LeSeulArtichaut (May 14 2020 at 18:01, on Zulip):

Oh, I amended my commit to add documentation to the variants of Prioritize but I didn't push it :face_palm:

Santiago Pastorino (May 14 2020 at 18:05, on Zulip):

@LeSeulArtichaut it should be ok now

LeSeulArtichaut (May 14 2020 at 18:06, on Zulip):

Just a last question: why do we need to call .to_owned() on the issue labels if we only iterate through them?

Santiago Pastorino (May 14 2020 at 18:07, on Zulip):

because it's wrong :)

Santiago Pastorino (May 14 2020 at 18:07, on Zulip):

was copy + paste

LeSeulArtichaut (May 14 2020 at 18:08, on Zulip):

I can't blame you, we all do the same :innocent:

Santiago Pastorino (May 14 2020 at 18:16, on Zulip):

last thing

Santiago Pastorino (May 14 2020 at 18:16, on Zulip):

going to change this

Santiago Pastorino (May 14 2020 at 18:16, on Zulip):
-                            if !issue_labels.iter().any(|l| glob.matches(&l.name))
+                            if issue_labels.iter().all(|l| !glob.matches(&l.name))
Santiago Pastorino (May 14 2020 at 18:16, on Zulip):

:P

Santiago Pastorino (May 14 2020 at 18:16, on Zulip):

!any seems silly

lcnr (May 14 2020 at 20:29, on Zulip):

:heart: https://github.com/rust-lang/rust/pull/71891

lcnr (May 14 2020 at 20:33, on Zulip):

^ realised that it's actually not exactly the same :shrug:

Santiago Pastorino (May 14 2020 at 21:01, on Zulip):

oh wow

Santiago Pastorino (May 14 2020 at 21:01, on Zulip):
if !associated_types.values().any(|v| !v.is_empty()) {
Santiago Pastorino (May 14 2020 at 21:01, on Zulip):

:S

Santiago Pastorino (May 14 2020 at 21:02, on Zulip):

it's like triple negation :)

Santiago Pastorino (May 20 2020 at 14:53, on Zulip):

@LeSeulArtichaut @simulacrum thoughts about this ...

Santiago Pastorino (May 20 2020 at 14:54, on Zulip):
diff --git a/src/config.rs b/src/config.rs
index cf73536..f4a91ee 100644
--- a/src/config.rs
+++ b/src/config.rs
@@ -82,6 +82,8 @@ pub(crate) struct PrioritizeConfig {
     pub(crate) prioritize_on: Vec<String>,
     #[serde(default)]
     pub(crate) priority_labels: String,
+    #[serde(default)]
+    pub(crate) exclude_labels: Vec<String>,
     pub(crate) zulip_stream: u64,
 }

diff --git a/src/handlers/prioritize.rs b/src/handlers/prioritize.rs
index 389c41b..74b3832 100644
--- a/src/handlers/prioritize.rs
+++ b/src/handlers/prioritize.rs
@@ -44,8 +44,13 @@ impl Handler for PrioritizeHandler {
                                 let issue_labels = event.issue().unwrap().labels();
                                 let label_name = &e.label.as_ref().expect("label").name;

-                                if issue_labels.iter().all(|l| !glob.matches(&l.name))
-                                    && config.prioritize_on.iter().any(|l| l == label_name)
+                                if issue_labels.iter().all(|l| {
+                                    !glob.matches(&l.name)
+                                        && config
+                                            .exclude_labels
+                                            .iter()
+                                            .all(|label_name| *label_name != l.name)
+                                }) && config.prioritize_on.iter().any(|l| l == label_name)
                                 {
                                     return Ok(Some(Prioritize::Label));
                                 }
Santiago Pastorino (May 20 2020 at 14:54, on Zulip):

this and ...

simulacrum (May 20 2020 at 14:54, on Zulip):

I don't have time right this moment to read and understand diffs I'm afraid

Santiago Pastorino (May 20 2020 at 14:54, on Zulip):
diff --git a/triagebot.toml b/triagebot.toml
index 2210a8ff8e6..1ee2aefef3f 100644
--- a/triagebot.toml
+++ b/triagebot.toml
@@ -38,4 +38,5 @@ label = "ICEBreaker-Cleanup-Crew"
 label = "I-prioritize"
 prioritize_on = ["regression-from-stable-to-stable", "regression-from-stable-to-beta", "regression-from-stable-to-nightly"]
 priority_labels = "P-*"
+exclude_labels = ["T-infra", "T-release", "T-rustdoc"]
 zulip_stream = 227806
LeSeulArtichaut (May 20 2020 at 14:54, on Zulip):

The TOML format seems good

LeSeulArtichaut (May 20 2020 at 14:55, on Zulip):

Reading the logic...

Santiago Pastorino (May 20 2020 at 14:55, on Zulip):

@simulacrum short summary is ... on regressions skip if the issue is labelled with ["T-infra", "T-release", "T-rustdoc"]

Santiago Pastorino (May 20 2020 at 14:55, on Zulip):

if you agree with the idea I just send a PR and we can all check the diff there at some point async

simulacrum (May 20 2020 at 14:55, on Zulip):

hm I guess, though in practice I think we currently do rely on compiler pings to some extent

simulacrum (May 20 2020 at 14:55, on Zulip):

but seems fine

LeSeulArtichaut (May 20 2020 at 14:55, on Zulip):

@Santiago Pastorino I think we should merge the priorities and exclude labels

Santiago Pastorino (May 20 2020 at 14:56, on Zulip):

into a pattern

Santiago Pastorino (May 20 2020 at 14:56, on Zulip):

yeah, makes sense I guess :)

LeSeulArtichaut (May 20 2020 at 14:56, on Zulip):

A list of pattern I'd say

Santiago Pastorino (May 20 2020 at 14:56, on Zulip):

yep

LeSeulArtichaut (May 20 2020 at 14:57, on Zulip):

So you'd have:

exclude_labels = ["P-*", "T-infra", "T-release", "T-rustdoc"]
LeSeulArtichaut (May 20 2020 at 14:57, on Zulip):

Which IMO is cleaner :D

LeSeulArtichaut (May 20 2020 at 14:57, on Zulip):

Also I think we'll need to update the documentation for the feature

LeSeulArtichaut (May 20 2020 at 14:57, on Zulip):

In the triagebot wiki

LeSeulArtichaut (May 20 2020 at 14:57, on Zulip):

But that's a nit :slight_smile:

Santiago Pastorino (May 20 2020 at 14:59, on Zulip):

yeah agreed

Santiago Pastorino (May 20 2020 at 14:59, on Zulip):

I've done exactly that

LeSeulArtichaut (May 20 2020 at 14:59, on Zulip):

:+1:

Santiago Pastorino (May 20 2020 at 15:00, on Zulip):

was trying to see if you both would notice this to take a short cut :joy:

LeSeulArtichaut (May 20 2020 at 15:00, on Zulip):

So this was a test? xD

Santiago Pastorino (May 20 2020 at 15:00, on Zulip):

:P

Santiago Pastorino (May 20 2020 at 15:00, on Zulip):

kidding kidding :)

Santiago Pastorino (May 20 2020 at 15:17, on Zulip):

sending PRs

Santiago Pastorino (May 20 2020 at 15:17, on Zulip):

first #72385

Santiago Pastorino (May 20 2020 at 15:19, on Zulip):

and

Santiago Pastorino (May 20 2020 at 15:20, on Zulip):

triagebot#561

LeSeulArtichaut (May 20 2020 at 15:21, on Zulip):

Let's make you rewrite everything review your code :D

Santiago Pastorino (May 20 2020 at 15:21, on Zulip):

well I'm already changing a conditional which is doing repeated work :)

Santiago Pastorino (May 20 2020 at 15:23, on Zulip):

pushed and well we may prefer to extract some methods there

LeSeulArtichaut (May 20 2020 at 15:25, on Zulip):
.all(|l| !glob.matches(&l.name))

Again? x)

Santiago Pastorino (May 20 2020 at 15:25, on Zulip):

lol

Santiago Pastorino (May 20 2020 at 15:26, on Zulip):

well to me the problem is any

Santiago Pastorino (May 20 2020 at 15:26, on Zulip):

that means all are different

Santiago Pastorino (May 20 2020 at 15:26, on Zulip):

that would be !any

Santiago Pastorino (May 20 2020 at 15:26, on Zulip):

I think I prefer all(not(something) than not(any(something))

Santiago Pastorino (May 20 2020 at 15:27, on Zulip):

the problem to me is using not(any(not(something))) or stuff like that

LeSeulArtichaut (May 20 2020 at 15:27, on Zulip):

I'm not the one maintaining the codebase, I'll leave reviewing the code quality to simulacrum :innocent:

Santiago Pastorino (May 20 2020 at 15:27, on Zulip):

my rule is kind of use any if it's straight, anyway this case may be better done with just 2 loops

Santiago Pastorino (May 20 2020 at 15:27, on Zulip):

let me do it that way

LeSeulArtichaut (May 20 2020 at 15:28, on Zulip):

Well I think we should always write code so it sounds good to us humans, right?

LeSeulArtichaut (May 20 2020 at 15:28, on Zulip):

Anyway

Santiago Pastorino (May 20 2020 at 15:35, on Zulip):
                        if config.prioritize_on.iter().any(|l| l == label_name) {
                            let mut prioritize = false;

                            for label in event.issue().unwrap().labels() {
                                for exclude_label in &config.exclude_labels {
                                    match glob::Pattern::new(exclude_label) {
                                        Ok(exclude_glob) => {
                                            prioritize = !exclude_glob.matches(&label.name);
                                        }
                                        Err(error) => {
                                            log::error!("Invalid glob pattern: {}", error);
                                        }
                                    }

                                    if !prioritize {
                                        break;
                                    }
                                }
                            }

                            if prioritize {
                                return Ok(Some(Prioritize::Label));
                            }
                        }

Santiago Pastorino (May 20 2020 at 15:35, on Zulip):

that more imperative way may be easier to read

LeSeulArtichaut (May 20 2020 at 16:03, on Zulip):

Either that or leaving comments (you know, those lines with // :laughing:)
EDIT: or both? x)

Santiago Pastorino (May 20 2020 at 16:42, on Zulip):

adding comments is for the weak :joy:, I hope nikomatsakis is not reading this :slight_smile:

Santiago Pastorino (May 20 2020 at 16:43, on Zulip):

srsly, agree :)

lcnr (May 20 2020 at 16:43, on Zulip):

just make the code self explanatory

Santiago Pastorino (May 20 2020 at 16:44, on Zulip):

@lcnr :), well this code is not particularly hard

Santiago Pastorino (May 20 2020 at 16:45, on Zulip):

I can extract a function needs_prioritization or something and would be way more readable but I'm not sure if all this is worth investing a lot of time :)

lcnr (May 20 2020 at 16:46, on Zulip):

:laughing: Not saying it's needed here

Santiago Pastorino (May 20 2020 at 17:01, on Zulip):

:)

Santiago Pastorino (May 20 2020 at 17:04, on Zulip):

added a comment anyway

LeSeulArtichaut (May 20 2020 at 17:57, on Zulip):

Hey @nikomatsakis, please check 8 comments above x)

Last update: Jun 05 2020 at 22:10UTC