webhook_features: Request review from stale reviewers

[aureleoules]

Attempt to fix #11.

This PR will make DrahtBot request reviews from stale reviews after every new push (those who left an ACK on a previous commit).

Currently in draft because this PR depends on https://github.com/XAMPPRocky/octocrab/pull/277.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

[aureleoules]

Rebased

[maflcko]

Personally I like the delayed ping, but I'll wait for others to chime in here

[aureleoules]

Personally I like the delayed ping, but I'll wait for others to chime in here

I don't really mind delaying it but by how long? 10-15mins or days?

[maflcko]

"a few days" was suggested in https://github.com/MarcoFalke/DrahtBot/issues/11#issuecomment-1326815040, so maybe a week?

[aureleoules]

A delay of days/week would require DrahtBot to have a stored state in case it crashes or just needs to be updated. I think it's a bit of a hassle but if others think it's worth it then okay.

[maflcko]

It could also be done "async" (like the :-1: feature), on the next (unrelated) update.

Another alternative would be to store the state in the comment (maybe inside a html comment), and then run another process to fetch those comments and run the update. Similar to inactive_rebase.

[kouloumos]

I think delaying is more appropriate. What I've seen from my behavior, if I've done a review, and someone later comes in with a thoughtful review, I'll wait a bit after the author has address those comments to give a chance to that reviewer to give his approval. Of course I can't wait for ever, but if that original reviewer has more comments on what has already been discussed/fixed, this might spare me one review cycle.

That said, my experience in the project is limited, and I would really like to hear from more experience contributors on how they are approaching this kind of stuff.

Apart from that, I have another idea (inspired by https://github.com/bitcoin/bitcoin/issues/26556) that might be a good match for this. It's a bit more complicate and it also makes the workflow more depended on GitHub, which I don't like, but I believe that it can be easily decoupled in the future.

The idea is using DrahtBot and Projects to display a list of all the stale reviews of each contributor. I haven't spend more than 10m looking into the implementation details of this but it seems doable. Whoever wants this functionality needs to create a Project and then give access to DrahtBot. So whenever there is a stale review, DrahtBot could go and add a record to the table. By also including a date, this could be used as reference for the delay.

[maflcko]

This was also suggested here https://github.com/MarcoFalke/DrahtBot/issues/11#issuecomment-1324013430, so maybe start a new thread with this idea?

[kouloumos]

This was also suggested here #11 (comment), so maybe start a new thread with this idea?

Thanks for pointing it out. I'll look a bit into the implementation details and do that.

[ajtowns]

A delay of days/week would require DrahtBot to have a stored state in case it crashes or just needs to be updated.

The list of requested reviewers is already stored state? I was thinking something like:

if last_push_date > now - 3 days:
    for who in ackers:
         review_up_to_date = False
         if who.last_comment_date > last_push_date + 5 minutes: review_up_to_date = True
         if who.acked_commit_hash = last_push_commit_hash: review_up_to_date = True
         if who.id in requested_reviewers and review_up_to_date:
             unrequest_review(who)
         elif who.id not in requested_reviewers and not review_up_to_date:
             request_review(who)

[maflcko]

I think the question was more about how to trigger the bot. If there is no webhook after 3 days, currently it won't be running.

Maybe there can be a slow-running background thread that cycles all pulls in a loop?

[DrahtBot]

🐙 This pull request conflicts with the target branch and needs rebase.

[maflcko]

I think async-only is better than nothing. I might pick this up unless your are still working on this?

[aureleoules]

I might pick this up unless your are still working on this?

Go ahead and pick this up.

[maflcko]

thx, did something in 0a9955b46b0a7f9277628094df28fb12f2cdffb1