iterative / py-template

Hypermodern Python Cookiecutter
http://cookiecutter-hypermodern-python.readthedocs.io/
MIT License
14 stars 7 forks source link

update-template: create a draft PR #104

Closed efiop closed 1 year ago

efiop commented 1 year ago

To emphasize that this doesn't require immediate attention and might not be a final complete solution (e.g. may contain unmerged rej files and stuff).

From the discussion with @omesser about notifications on update-template PRs that were hanging for a months or so.

skshetry commented 1 year ago

This feels like we are trying to game the system.

efiop commented 1 year ago

@skshetry Could you elaborate?

The PRs usually have .rej files, draft aslo prevents blind merging.

skshetry commented 1 year ago

The PRs usually have .rej files

There should not be any .rej files, we handle that with this github action. There are going to be conflicts, but that'll block the merge. And conflicts are going to be rare.

I don't see how this is any different than dependabot PRs. These PRs are not urgent, but they should not stay open for more than a month. Creating a draft PR will reduce visibility and merges from happening too.

The slack bot should be filtered to not send these kinds of notifications though, or change the threshold for these bot PRs.

efiop commented 1 year ago

@omesser Can that app be tuned to not pick up update template PRs?

omesser commented 1 year ago

we don't have such fine-grained control (we can exclude the repo though), but slack-bot completely aside - are those pull requests or drafts? if they should be indeed reviewed and merged (meaning their existence means intent) - then pull requests (priority aside), if they should be treated as draft until a human verifies their contents and need - drafts. I was under the impression this is was made a draft because it's the right thing to do, not to game the bot. Having stale PRs forever is just clutter and not good to have - regardless if it's man-made or bot-made. including dependabot - they are made to review and merge, not to sit stale

skshetry commented 1 year ago

Every PR should be verified by a human and merged, no? I don't think we should try preventing it with drafts.

That said, I understand the concern. We did have accidental merges (looking at dvc-oss 😄), and bot PRs (both template-update PRs/dependabot PRs) do remain open for a long time.

omesser commented 1 year ago

Every PR should be verified by a human and merged, no? I don't think we should try preventing it with drafts.

Actually, draft PRs are a mechanism exactly for the issuer to communicate to potentials reviewers that this is not ready (or doesn't) need a review yet (there's visibility, which is better than not opening anything and cooking the branch / changes locally) but a clear signal not to spend time on this. So, not a new concept I believe.

What @efiop wrote in the description seems very sensible to me - draft signals, ignore this for now - it's pending something / blocked on something. There's another human intervention point (converting draft PR to PR) when someone says "I now intend to merge this". From this point on it would be valid to "expect" this to be reviewed asap (within reason/capacity of course)

efiop commented 1 year ago

Let's try this out for now.