Open
Conversation
|
I can’t complete a real review yet because I can’t read the repo in this session: all shell commands fail with Please paste one of these, and I’ll review immediately:
Once you share that, I’ll return concise findings focused on correctness regressions, security, and missing tests/edge cases only. |
3 tasks
b3bdb16 to
cd2ef3c
Compare
creating a main() function which handles Click's exceptions and returns an integer, or exits. Per the documentation at * https://click.palletsprojects.com/en/stable/exceptions/ we need to catch click.Abort and click.ClickException, but BrokenPipeError was also added. Recast the cli() function as click_entrypoint() to help differentiate it from run_cli(). The latter one could even be renamed to run_repl()! Motivation: prompt_toolkit is already handling exceptions such as KeyboardInterrupt. We had two layers trying to handle exceptions transparently, and we want control-c to cancel pending queries very reliably. Let's start to untangle. This also exposes the arguments to be processed by Click, which is desirable in case we want to pre-process them. Example: "mycli -h" could print the helpdoc, instead of being interpreted as a missing hostname. Example: we could catch some issues with --password before running Click.
cd2ef3c to
bc703a6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Handle Click exceptions by hand, creating a
main()function which handles Click's exceptions and returns an integer, or exits.Per the documentation at
we need to catch
click.Abortandclick.ClickException, butBrokenPipeErrorwas also added.Recast the
cli()function asclick_entrypoint()to help differentiate it fromrun_cli(). The latter one could even be renamed torun_repl()!Motivation:
prompt_toolkitis already handling exceptions such asKeyboardInterrupt. We had two layers trying to handle exceptions transparently, and we want control-c to cancel pending queries very reliably. Let's start to untangle.This also exposes the arguments to be processed by Click, which is desirable in case we want to pre-process them. Example:
mycli -hcould print the helpdoc, instead of being interpreted as a missing hostname. Example: we could catch some issues with--passwordbefore running Click.Checklist
changelog.mdfile.AUTHORSfile (or it's already there).