Skip to content

Conversation

@hauntsaninja
Copy link
Collaborator

See #8157 for an example

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Nice, thanks

@JelleZijlstra JelleZijlstra merged commit a9fb8c5 into python:master Jun 25, 2022
@hauntsaninja hauntsaninja deleted the stubsabotdev branch June 25, 2022 16:49
If stubtest fails for this PR:
- Leave this PR open (as a reminder, and to prevent stubsabot from opening another PR)
- Fix stubtest failures in another PR, then close this PR
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have never understood these instructions. Why not just fix the PR by pushing commits directly to it, and merge when stubtest passes again?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that seems to be the more common rhythm we're settling into.

Copy link
Collaborator Author

@hauntsaninja hauntsaninja Jun 26, 2022

Choose a reason for hiding this comment

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

It's a good question. It's because of what the current code will do if we run it automatically on a regular basis (rather than me running it manually in batches). stubsabot.py just makes a commit off of master and pushes it. master changes regularly, so these pushes would fail. Rather than failing, silently doing nothing, or some more complicated logic that checks what's on the remote, it currently force pushes to the remote:

subprocess.check_call(["git", "push", "origin", branch_name, "--force-with-lease"])

So this means it could undo changes we made.

But thinking about it a little bit more, silently doing nothing might not be that bad an option?

The other reason for these instructions is in theory non-maintainers could contribute (and they might benefit from instructions more so than maintainers).

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