Skip to content

[stubsabot] add script #8035

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Jun 18, 2022
Merged

[stubsabot] add script #8035

merged 13 commits into from
Jun 18, 2022

Conversation

hauntsaninja
Copy link
Collaborator

@hauntsaninja hauntsaninja commented Jun 8, 2022

This implements the suggestion in #7537 (comment) (and builds on some of the code from that PR). The proposed workflow here is we automatically get PRs updating the version in METADATA.toml. If stubtest passes, we can move on with our lives with reasonable confidence the stubs are okay (especially once we get rid of --ignore-missing-stub for third party stubs).

I haven't yet tested it end to end or tried getting it to work inside Github Actions, figured this was about the right point in time to discuss.

@JelleZijlstra
Copy link
Member

Great idea!

@hauntsaninja
Copy link
Collaborator Author

Okay, added automatic obsoletion to this. I also did a one time pass to manually make PRs for stubs that are obsolete, so that we get the correct version numbers.

@hauntsaninja
Copy link
Collaborator Author

Thoughts on next steps here? My plan is to open small batches of these so people get a sense for the workflow, but let me know if stubsabot is annoying and I won't.

@JelleZijlstra
Copy link
Member

Sounds like a good plan.

@AlexWaygood
Copy link
Member

What should we do for stubsabot PRs like #8081, where it's clear that a little bit of work needs to be done to update the stub? Should we leave those PRs open until one of us has time to do the work? Close the PR, and open an issue to remind us that the stubs for that package are out of date?

@hauntsaninja
Copy link
Collaborator Author

One possible workflow:

  • Leave the PR open as a reminder. Doing so will also prevent the current stubsabot code from opening another PR. I don't have much experience with dependsabot, but I think folks commonly leave those open for a bit as well.
  • When one of us has the time to fix it, we just open another PR, merge, and close the stubsabot PR.

Feels like that could be pretty manageable in the steady state (not unlike the daily stubtest when people release new versions that match the existing specifier in METADATA.toml, but break stubtest). And the current backlog we can handle on our own time, either by not running stubsabot in CI for a while or by using an allowlist or something.

@AlexWaygood
Copy link
Member

AlexWaygood commented Jun 14, 2022

  • Leave the PR open as a reminder. Doing so will also prevent the current stubsabot code from opening another PR. I don't have much experience with dependsabot, but I think folks commonly leave those open for a bit as well.

If you close a Dependabot PR, he (she? it?) folornly leaves a message saying he won't contact you again until there's a new release, e.g. python/devguide#784 (comment). (Not saying I want that level of advanced automation from stubsabot, just for reference!)

or by using an allowlist or something.

I think I prefer the idea of an allowlist. I'm not a massive fan of having a large number of open PRs with red crosses next to them, but an allowlist we can work through in our own time. It would also be a nice thing to point contributors towards in CONTRIBUTING.md. (Want to help out? Here's a handy list of all our stubs which we know to be somewhat out of date!)

Outside contributors might have a hard time contributing towards stubsabot PRs, on the other hand, since they won't be able to push towards stubsabot's branch. (They can open a new PR, of course, but that might not be obvious.)

@hauntsaninja hauntsaninja changed the title [stubsabot] proof of concept [stubsabot] add script Jun 14, 2022
@hauntsaninja
Copy link
Collaborator Author

Okay great. Updated plan:

  • The basic script is ready for review and merge. Merging now will make review of future tweaks easier.
  • I'll continue to manually open small batches of these so we can see what other workflow issues crop up. I'll make sure that we have at most three open stubsabot PRs failing CI.
  • I'm hopeful we can close the backlog quickly, if that proves not to be the case and we want to make this run automatically, we'll use an allowlist for the backlog.
  • At some point, make this run in CI (probably on a weekly basis).

@hauntsaninja hauntsaninja marked this pull request as ready for review June 14, 2022 23:56
@AlexWaygood
Copy link
Member

Okay great. Updated plan:

  • The basic script is ready for review and merge. Merging now will make review of future tweaks easier.
  • I'll continue to manually open small batches of these so we can see what other workflow issues crop up. I'll make sure that we have at most three open stubsabot PRs failing CI.
  • I'm hopeful we can close the backlog quickly, if that proves not to be the case and we want to make this run automatically, we'll use an allowlist for the backlog.
  • At some point, make this run in CI (probably on a weekly basis).

Okay great, I'm on board — thanks for doing this!

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few nits, but this basically looks great! (I haven't tried the script out myself, but it all looks reasonably self-explanatory, and the PRs you've been posting look solid.)

hauntsaninja and others added 5 commits June 17, 2022 00:21
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@hauntsaninja hauntsaninja merged commit c31baed into python:master Jun 18, 2022
@hauntsaninja hauntsaninja deleted the stubsabot branch June 18, 2022 04:52
@hauntsaninja
Copy link
Collaborator Author

hauntsaninja commented Jun 18, 2022

Thanks! Merged to make review of future changes easier and since this seems like a passable prototype. Also because I named my branch poorly and it conflicts with the prefix in my fork. Still very interested in feedback (and not attached to doing this if people have concerns), so feel free to post here or open an issue for follow ups as they come up :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants