-
Notifications
You must be signed in to change notification settings - Fork 16
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
Add proper CLI, supports upload, download and repl commands #30
Conversation
ctx.invoke(repl) | ||
|
||
@cli.command(short_help="Download a file from crow") | ||
def download(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the repl interaction (p
) and the crow command (^^p
) it might make more sense to call this print
instead of upload
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think ^^p
is 'download' right? the main usage of the download script is that you arrow it to a file druid download > myscript.lua
. this style of behaviour makes it feel more like a 'download' than a 'print' to me. that said, you are correct that what druid does is just 'print'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was just thinking that for naming consistency it might make sense to call it print
.
Also looking at your example for me it seems to support that print
might be a better name in that the command itself just prints to stdout, it doesn't write to a file, which is what I'd normally associate with downloading.
On the other hand if you curl a file then it goes to stdout as well and I'd call that downloading as well.
Hmm 🤔
Naming is hard...
375beee
to
9c9e0e6
Compare
Supports upload, download and repl subcommands Defaults to the REPL if no subcommand is passed
9c9e0e6
to
edb13f3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I'm happy to roll with the current iteration for now.
Let me test it on all 3 computers, then merge this afternoon!
ctx.invoke(repl) | ||
|
||
@cli.command(short_help="Download a file from crow") | ||
def download(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think ^^p
is 'download' right? the main usage of the download script is that you arrow it to a file druid download > myscript.lua
. this style of behaviour makes it feel more like a 'download' than a 'print' to me. that said, you are correct that what druid does is just 'print'.
For discussion with @csboling
It support three subcommands:
download
,repl
,upload
When no subcommand is given it default to starting the REPL as before.
There's one thing that doesn't work/I'm not sure is possible: passing a filename to the REPL without using the
repl
subcommand, so something like:This uses Click for creating the CLI. I've found that it works pretty well for defining well designed CLIs, it's built-in support for (sub)commands is the main thing that's quiet nice, it matches well with CLIs that need to do multiple things like druid.
Note that I've just moved the existing code verbatim from the upload and download scripts without changing anything.
Some cleanup/reordering should happen because there's currently logic on how to interact with the module embedded in the cli part, this should obviously be part of the library. But I'd suggest to do that in a separate PR.
/cc @trentgill
Fixes #24
Some usage examples: