search

rust-lang / triagebot

Automation/tooling for Rust spaces
https://triage.rust-lang.org
Apache License 2.0
172 stars 73 forks source link

Add cutoff env var for PRs WRF from T-compiler #1593

Closed apiraino closed 2 years ago

apiraino commented 2 years ago

Very small patch but very useful for me when drafting the T-compiler weekly agenda. The default cutoff value (5) for listing the PRs waiting for a review from the T-compiler is often not enough, I need to dig deeper in this github query.

Having a dynamic cutoff value is for me very helpful. Default value (5) is as before when no override is provided.

r? @Mark-Simulacrum

thanks!

nikomatsakis commented 2 years ago

Seems fine, but couldn't we do this more elegantly-- e.g. as part of the template or something, so you don't have to set the environment variable?

Mark-Simulacrum commented 2 years ago

Can you say more about the motivation here? I'm definitely not really happy with an arbitrary environment variable -- seems hacky -- but I'm not sure I understand yet how a dynamic value is helpful for you, and would like to know more.

apiraino commented 2 years ago

Sure, no problem :)

@Mark-Simulacrum so the thing I am trying to solve is that there is a GraphQL query pulling the first 100 PRs assigned to T-compiler waiting for review. Then the first 5 elements are extracted, all the rest is discarded. This is my understanding of the implementation.

Now, these PRs tends to stay in s-waiting-for-review for some time, not always the reviewer can immediately work on them, so the following week I would end up with the same 5 results in the T-compiler agenda. In order to rotate the PRs on the agenda a bit I need to skip the first results and go down the list. This is why printing a longer list helps me doing the agenda.

I could have hardcoded a 20 instead of the 5 but I'm not too fond of hardcoded values.

An alternate implementation can be to remove the .take(5) and try implementing the filter at the template level, like @nikomatsakis suggests (Tera, the templating engine we use, supports for loops with indexes).

I didn't think of that option (so thanks @nikomatsakis!) so if it works, would that make everyone happy?

Mark-Simulacrum commented 2 years ago

It seems like we should always dump all the PRs, or perhaps randomly shuffle them and pick 5? I'm not sure yet why you want a dynamic configuration here, vs. just increasing it :)

apiraino commented 2 years ago

It seems like we should always dump all the PRs, or perhaps randomly shuffle them and pick 5? I'm not sure yet why you want a dynamic configuration here, vs. just increasing it :)

for all purposes and intents, simply increasing .take(5) to .take(50) would be fine for me, if you prefer this way.

w.r.t. randomly picking would probably not help because (and I forgot to mention that) I need them sorted by "last activity date" (a metric calculated by looking at the latest comment from the PR reviewer/author).

Mark-Simulacrum commented 2 years ago

I think GitHub has some support for last activity date, though it typically fails to reflect what I actually want :)

I would be happy to s/5/50, that seems totally fine to me. I'm not a fan of the environment variable to configure things, that's all.

apiraino commented 2 years ago

@Mark-Simulacrum I have amended this patch as per our discussion.

let me know if it's fine, thanks!