Skip to content
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

Added command-line options --ask and --save (and --version) #15

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pdkl95
Copy link

@pdkl95 pdkl95 commented Aug 10, 2013

Option --ask asks for a positive 'y' or 'yes' response before uploading.
Option --save skips uploading entirely and saves locally.

As this adds several new options, the entire option-parsing
process was rewritten to use OptionParser.

For completeness, support for --version was added as well.
This change also moved the version number out of the .gemspec,
into "lib/showterm/version.rb".

Option --ask asks for a positive 'y' or 'yes' response before uploading.
Option --save skips uploading entirely and saves locally.

As this adds several new options, the entire option-parsing
process was rewritten to use OptionParser.

For completeness, support for --version was added as well.
This change also moved the version number out of the .gemspec,
into "lib/showterm/version.rb".
@poulson
Copy link

poulson commented Aug 10, 2013

It would be nice to have the option of both uploading and saving (for backup purposes). Is there an easy way to download the recorded session from the generated showterm.io link?

@pdkl95
Copy link
Author

pdkl95 commented Aug 10, 2013

@poulson Such an option would be only a minor change to my patch. There's several ways that could work, though.

Currently, my -s/--save option is the same call to Main#die_politely that happens after an upload failure or when you cancel an -e timing edit. This means you can immediately upload the saved files with --retry.

On the other hand, it would be trivial to wrap that up in another option. What would be the "least surprising" behavior, though: should --save only save, with a --save-and-upload alternative? Should --save simply force on saving a local copy, with something like --skip-upload used with it to get my patch's current behavior?

This brings up the question, too, of if it should be possible to name your save file. [and another concern about the generated files themselves, but that's another feature entirely that I'll discuss in another ticket (#19)]

While I'm not sure it's needed (or desired?), here's a possible larger set of options for discussion:

  • --o <prefix> / --output-preifx <prefix>
    Prefix (including path, if given) for the saved recording's files (instead of the current "/tmp/showtime.$PID").
    The suffixes .script and .times would then be added automatically.
  • -s / --save
    Save locally, using either a default name or the prefix given with --output-prefix
  • -U / --skip-upload
    Disable uploading. Using this without --save could be considered an error.
  • -u / --upload
    optional It might be nice to rename --retry to this, as it wouldn't be "retrying" an upload
    that was previously skipped. Changing the current interface without leaving the original in as an alias would probably annoy at least some people.

Opinions? Additions/Modifications? I'd be happy to add these kind of changes, if needed. I'm fine leaving it as-is, too.

@telemachus
Copy link

Currently, my -s/--save option is the same call to Main#die_politely that happens after an upload failure or when you cancel an -e timing edit.

@pdkl95 I don't have strong feelings about some of your questions, but I noticed this. I think it's a big mistake for something like -s/--save to call Main#die_politely. That causes the program to exit with a non-zero status, indicating failure. And a program should never exit with a failure status if it's done its job correctly.

Maybe the easiest way to handle this would be to add an (optional?) parameter to die_politely. The parameter (if given) would be an exit status. If not given, it defaults to 1 (the current choice). Then in other places you could still use die_politely and pass a 0 to indicate success.

@pdkl95
Copy link
Author

pdkl95 commented Aug 11, 2013

(>_<) I have no idea why I missed the exit code. You're entirely correct, of course.

I'm fixing it now. Adding an arg for the exit code is easy, but... now that I'm doing it, I think I'll also split this into separate functions. That way #die_politely can always mean "die" (with and error) as it did before. The split off #save_politely then only deals with the actual save+message.

refactored #die_politely tow functions - #die_politely
for the traditional error cases that call #exit, and
a new #save_politely that only does the save w/ message.

cleaned up a few of the nearby print statements for clarity.
@poulson
Copy link

poulson commented Aug 12, 2013

I would propose, if no command-line arguments are given, for two questions to be asked: "save a local copy?" and "upload?". It would be nice for all four combinations to be possible. There are also many usecases where one might want to go back and download a copy from the website (for example, perhaps someone else uploaded it).

@telemachus
Copy link

I'd would vote against the last proposal. Normal Unix-like rules are "no arguments at all" -> show help menu or just start. Which happens depends on the application. "Chatty" applications that ask for too many responses are generally less useful, since they don't work as well in pipes or scripts.

Just my two cents.

@ConradIrwin
Copy link
Owner

@pdkl95 Hey, nice patch! What's the --save argument for? (I don't really see the use-case, and it seems like a confusing choice for a user to have to make and a pretty bad implementation, saving files into /tmp is basically the same as not saving them).

@poulson I want typing showterm to "just work" by default, even apparently simple questions make the user experience way less smooth than having everything automatic.

That said, I would like to implement downloading (shouldn't be too hard, but it might be worth switching from a two-file to a one-file format first, see #19) and deleting.

@poulson
Copy link

poulson commented Aug 12, 2013

@telemachus @ConradIrwin I agree with your point, but I honestly don't care whether or not the configuration is exposed via prompts or via command-line options. The point is that it would be nice to be able to handle all four combinations of saving/not-saving and uploading/not-uploading.

@pdkl95
Copy link
Author

pdkl95 commented Aug 14, 2013

@telemachus I agree that the unix tradition of "silent on success, noisy on error" is something to maintain when possible, and the other use cases can easily be handled by options. The possible exception to this I could see (note: not actually recommending this!) is, given the auto-upload feature might be unexpected for someone trying it for the first time, a warning that is shown on the first run might be useful. Such a warning is probably overkill in this case, where the uploading was kind of the whole point.

@ConradIrwin The --save feature is to allow the upload to be deferred. I initially wanted it to inspect what I would be uploading when initially trying this out. A better reason, though, is to allow for recording without internet access. Ideally, it should also allow for naming the saved file(s), of course; I was trying to keep these patches as minimal as possible for the moment, though I agree with @poulson that it would be nice to have some sort of clear, obvious support for all four [{not-,}saving {not-,}uploading] possibilities. This is what I was trying to bring up in comment at the top of this thread.

I'll see if I can upload my branched version I've been poking at later, if I can finish a couple things, that both includes a possibility for the single-file idea in #19, and has a larger rework of the command-line options. That might serve as a useful reference for discussion.

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