--update-all and --interactive #96
Conversation
|
@mythmon I don't need an in-depth review but can you take a look? Basically, the way I test it is: python hashin.py -r ~/songsearch/requirements.txt -i -u --dry-runDo you like the interactive prompt? Is it working? Anything else obvious you see? |
There was a problem hiding this comment.
This seems good to me. The UI makes sense, and worked on the requirements file I tried (one of hashin's actually). The only way I could break it was by running yes | hashin ..., which repeatedly answered an invalid answer and eventually crashed Python. Not a big deal.
hashin.py
Outdated
| first_interactive = False | ||
| continue | ||
| first_interactive = False | ||
| except InteractiveAll: |
There was a problem hiding this comment.
My general rule is to only use exceptions for exceptional cases. "All" and "Quit" don't seem exceptional to me. I think I would make interactive_upgrade_request return a set of constants for all the cases, instead of returning a boolean in some cases and raising exception in others.
There was a problem hiding this comment.
Yeah, I tend to agree. ...in hindsight. I think I fell into the trap of doing it like this because I had originally not planned to use "all" and "quit" and when I added those it didn't match the way I called the interactive_upgrade_request function.
| clear_line() | ||
| print_line(True) | ||
| return True | ||
| return ask() |
There was a problem hiding this comment.
It might be better to put this in a loop instead of recursing. I think that makes the repetition easier to read.
There was a problem hiding this comment.
Point taken. I can see how that could be used to achieve the same thing. But I kinda like it as a function like that. Perhaps it fits my brain better.
hashin.py
Outdated
| sys.stdout.write("\033[K") # Clear to the end of line | ||
|
|
||
| def ask(): | ||
| answer = input("Update? [Y/n/a/q]: ").lower().strip() |
There was a problem hiding this comment.
As a nifty improvement, it would be cool to add "?" to the list of options, and if ? or an invalid choice is given, it prints a longer version of this prompt out, like:
y - Include this upgrade (default)
n - Skip this upgrade
a - Include this and all following upgrades
q - Skip this and all following upgrades
? - Print this help
You can see a similar UI in git add -i, which prompts you to include, skip, edit, etc chunks of files.
|
Sigh. I've stumbled on a bug when working on that feedback. In an old I already had some code in place to deal with case insensitivity. E.g. if you had What should happen in this case, if I chose to press "Y" to upgrade to that Back to the drawingboard a bit. |
|
@mythmon Wanna take a last look? The recent changes are quite insignificant. In particular, I discovered a bug in my first attempt where if you had The other thing I changed was that I add the "?" option to the prompt. Thank you. Also, I changed it so that if you do say "A" ("all") it will "keep the interactive", but not ask any more questions. That way you can clearly see which packages can be updated and what they get updated to. |
Fixes #90