Skip to content

Conversation

svalentin
Copy link
Collaborator

If misc/sync-typeshed.py fails to apply a cherry pick, it just fails. Let's try to give the user a chance to manually merge the CP and continue with the script.
This should block for user input only in cases where stdin is a tty. So automation should continue failing. (but the only way to test is by running it)

@svalentin
Copy link
Collaborator Author

@JukkaL or @AlexWaygood any opinions about something like 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.

This is a great quality-of-life improvement, thank you! Just one very small suggestion:

f"Commit {commit} failed to cherry pick."
" In a separate shell, please manually merge and continue cherry pick."
)
input("Did you finish the cherry pick?")
Copy link
Member

Choose a reason for hiding this comment

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

Trying the script out locally, my naive expectation was that I was required to either type "y" or "n" to continue. I typed in "y", which meant that the line on the terminal was

Did you finish the cherry pick?y

Looking at the script again, you obviously don't need to type anything at all. But it still might be nice to have a space at the end here so that it doesn't look so weird if a user does input something 😆

Suggested change
input("Did you finish the cherry pick?")
input("Did you finish the cherry pick? ")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, It was a quick change on my part to get feedback if it's useful. I'll actually check the return of input 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
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.

Nice!

@svalentin svalentin merged commit 59ba429 into python:master Jul 4, 2023
@svalentin svalentin deleted the sync-typeshed-cp-fail branch July 14, 2023 11:36
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.

2 participants