Do not apply labels in autolabel when opening a draft PR

[Kobzol]

After #1963, it doesn't make sense to apply S-waiting-on-review to PRs opened as a draft. According to https://github.com/search?type=code&q=org%3Arust-lang+%22new_pr+%3D+true%22, this option is only ever used for adding S-waiting-on-review in the rust-lang org, so I don't think that we need to complicate the settings by adding some "new_nondraft_pr" setting or something like that.

[Kobzol]

Actually, this won't work on its own, because when a draft PR is opened, it won't get S-waiting-on-review, and once it's converted from draft, it will get assigned someone, but it won't have that label.

Maybe we should modify autolabel to treat both "opened as non draft" and "converted from draft" as triggering new_pr = true?

[apiraino]

I'm thinking if it makes sense applying any kind of post-processing to draft PRs. Is there anything the triagebot needs to do on drafts?

Would it be too radical to set a global is_draft() gate on all handlers and just skip everything?

[Kobzol]

We might go in that direction, yeah. In theory we could just ignore draft PRs completely, and the first time they are converted from draft to open, we could treat it in triagebot as them opening at that very moment. But that would probably require some larger changes.

[Urgau]

Maybe we should modify autolabel to treat both "opened as non draft" and "converted from draft" as triggering new_pr = true?

Make sense to me.

[Kobzol]

Ok, I changed the logic, let me know what do you think.

[apiraino]

Suggested change

	#[derive(Debug, Default, serde::Deserialize, serde::Serialize)]
	struct AutolabelState {
	new_pr_labels_applied: bool,
	}
	#[derive(Debug, Default, serde::Deserialize, serde::Serialize)]
	struct AutolabelState {
	has_new_labels: bool,
	}

?

[Kobzol]

That sounds a bit confusing/not fully accurate to me, because these labels might not necessarily exist on the PR right now, we just remember that we have already tried to apply the new_pr labels already. I added a comment to explain this, let me know what you think.

[apiraino]

we just remember that we have already tried to apply the new_pr labels already

I see. So in plain English it would be:

This flag registers if a PR was previously labeled with the predefined set of labels we apply to all new PRs"

Is that correct? Then maybe already_autolabeled (or similar)? But I agree, a bit hard to summarize in a short variable name.

(btw, this flag purpose would be more clearer if named is_new_pr or similar)

[Kobzol]

• new_pr_already_autolabeled
• have_new_pr_labels_been_applied

Do you like any of these? 😆 I think that the comment is enough, we probably can't find a completely perfect name for this flag.