Add "headless" mode to metis_client #724
Conversation
|
Sweet, the tests all passed, so this isn't breaking the original behavior. @corps I'm not sure how to go about testing this stuff without duplicating almost all of |
metis/bin/metis_client
Outdated
| attr_reader :options | ||
|
|
||
| def initialize(shell) | ||
| @headless = ENV['METIS_headless'] == 'true' ? true : false |
There was a problem hiding this comment.
First, thanks so much for putting in a PR -- this is awesome!
super-nit: is the convention for environment variables to be in all caps? i.e. METIS_HEADLESS
There was a problem hiding this comment.
haha yeah that's a good point.... it should be ALL_CAPS. Will fix in the next version!
|
This is a great idea, thanks for writing this. Two points:
|
|
Hmm. Looks like i was mistaken about point 2 and lines are NOT truncated. Maybe i recalled wrong from a previous use of the command. regarding using |
|
Also, just as a quick question, do you guys want me to squash + rebase + force push, or can i have multiple commits on the branch? The lead on my old collaborative repo would make us do that so the commit history would be super clean |
We are not usually picky. Multiple commits is fine, squashing and rebasing is fine, merging in master is fine. |
1. Add `--headless` as a client argument that makes some changes to output including increasing line size in ls (via increasing term size to an arbitrarily large number, 1000), and updating the get/put progress to only write 1 line on completion of the action. 2. rename `progress` to `print_progress` and refactor to move print statements into the function. 3. Add a "verbose" argument to print_progress to implement (1) above 4. Refactored all calls to progress to appropriately implement print_progress 5. Given `Nuke` is a destructive action, the print behavior is modified to print one line per file processed with `puts` instead of `print` so the output buffer in headless mode isn't corrupted by the carriage returns.
Modify `MetisShell.new` calls to reflect new `client_args` addition to `initialize`
Reverse changes to tests.
arkal
force-pushed
the
aar/headless-metis-client
branch
from
b8bffed to
70b362b
Compare
Readline uses [0, 0] for terminal size in a script, and this will truncate the echo-ed input command, making the logfile annoying to read/parse. Explicitly setting the col width to a large number in a scripted mode will fix this issue
|
Looks like it works. I've modified the code to use STDOUT.isatty, and i've just pushed a small fix that alters Readline's perception of terminal size to prevent truncation of the echo-ed input command, making for prettier outputs. This brings me back to my original question: How do you want me to test this? Given that the code is modified ever so slightly, I don't think i need unit tests for any of this but that may affect the code coverage and i don't know how important you guys think that is. |
|
Already loving the results, if i do say so myself. BEFORE AFTER |
| if STDOUT.isatty | ||
| print "Computing md5s - #{progress_bar(i.to_f / files.length)} - #{i} of #{files.length}\r" | ||
| else | ||
| puts "Computing md5s - #{progress_bar(i.to_f / files.length)} - #{i} of #{files.length}" |
There was a problem hiding this comment.
Absolutely, but like I said above, Nuke is a destructive operation so verbosity is good, no? This is just an md5 check though so it may be OK to only print completion
|
We're not super strict about test coverage percentages as a metric ... I think the general philosophy I take is to make sure that tests cover the important logic of the features I add, to prevent regression bugs. You could perhaps set STDOUT.isatty during test setup (not sure??), and verify the output format changes. |
|
Right that was an option, but i'd have to duplicate a large chunk of client_spec so i test bot with and without STDOUT.isatty ? |
|
Oh, I wouldn't duplicate all the tests in client_spec with / without STDOUT.isattty. I would probably just add one or two small tests that capture the core formatting changes. Kind of like these for set -e. I don't think it's possible to test upload see comment, but you could test download output to see if it matches the new, expected format...so like some version of this test, perhaps combined with the If that is still a bit unclear, let me know and we can pair or I can sketch out in more detail what some specs might look like... |
|
That makes sense. I can work on this later this week will ping when i have something working |
|
I'd be content with a test each covering get and ls. |
The current schema for printing output in
metis_clientis basedon a standard interactive terminal. This is not relevant when one
is attempting to automate multiple commands in one session.
For example,
that render terrible when piping output to a file
This PR implements a
--headlessflag tometis_clientthat modifies the behavior of metis_client slightly, to make forbetter output when the result is piped into a logfile.