Use wrappers around action methods to handle quiet/pretend invocation of commands

[nickmccurdy]

Here's an example of running grep say_status lib/homesick/actions.rb on homesick's source.

say_status 'git clone', "#{repo} to #{destination.expand_path}", :green unless options[:quiet]
say_status :exist, destination.expand_path, :blue unless options[:quiet]
say_status 'git init', '' unless options[:quiet]
say_status 'git init', 'already initialized', :blue unless options[:quiet]
say_status 'git remote', "add #{name} #{url}" unless options[:quiet]
# about 20 lines removed for brevity

The quiet and pretend options are often checked manually, which leads to fairly complex looking code.

Instead, I think it would be cleaner to use methods that handle quiet/pretend invocation for us. I vaguely remember seeing something in Thor's source code about a quiet? getter method that is used in say_status, but I couldn't figure out how to set this yet. It would be nice to use features built into Thor if there are any for this, but worst case we can add our own wrapper methods.

[JCook21]

I'd be in favour of removing all of the :quiet and :pretend options completely if possible. I added the capture-output library to the project recently for testing so suppressing output in tests can be done with 'Capture.stdout { CODE HERE }' or 'Capture.stderr { CODE HERE }'. You can also capture the result of the code block into a variable to test it. It means a little more work in the tests but I think it's worth it to keep the code cleaner.

[technicalpickles]

I'd be in favour of removing all of the :quiet and :pretend options completely if possible

Why is that? These options are for using homesick on the command line, not just for testing.

[JCook21]

@technicalpickles fair enough, my mistake.

[JCook21]

@technicalpickles I wonder if there's a way to make this more DRY though? I wonder if it could be refactored so that all calls to output messages could be sent to a single method and options checking could be done there instead?

[technicalpickles]

options is on the object Homesick::Actions is being mixed into, so feasibly could override say_status in Homesick to only call super if ! options[:quiet].

[JCook21]

That sounds like a good plan.

[nickmccurdy]

@technicalpickles I think this would work well, and I started some work on it. However, I noticed that while unless options[:quiet] is always used when calling say_status in actions.rb, this is not always the case in homesick.rb. Should we only override say_status to conditionally silence itself in Homesick::Actions, or is it safe to assume that say_status should always be used with unless options[:quiet]?

[technicalpickles]

Hard to say off hand. If the purpose of quiet is to make a command that executes quietly with no output, then all say_status should respect it. It looks like there's errors being displayed with that (ie say_status with :red).

[nickmccurdy]

If the purpose of quiet is to make a command that executes quietly with no output, then all say_status should respect it.

Agreed. Should we also do this for system and the pretend option?

By the way, I have most of this wrapper functionality implemented on my refactor branch now (I didn't make a PR for it yet).

Currently, I have a couple methods that handle the wrapping, and I have replaced references in the code to use them.

# Similar to say_status in Thor::Actions, but does nothing if the quiet
# option is enabled.
def smart_say_status(*args)
  say_status(*args) unless options[:quiet]
end

# Similar to system in Kernel, but does nothing if the pretend option is
# enabled.
def smart_system(*args)
  system(*args) unless options[:pretend]
end

They're fairly trivial and I don't really like their names, but I created these new methods at the time instead of overriding say_status and system directly because I was afraid of breaking invocations of say_status and system that didn't use unless at the end (so I wouldn't need to use the smart_* methods in cases that didn't have unless at the end of the line).

Do you want me to try overriding these methods (say_status and system) and removing my smart_* methods?

[JCook21]

@thenickperson sorry to but into the conversation but FWIW I'd just say to go ahead and override the methods. It would be cleaner and easier to maintain and shouldn't affect anything... (now I've jinxed it!)

[technicalpickles]

I think the only reason not to override the defaults would be those cases where we always say_status (like for displaying errors) or always system (uh, can't think of any offhand).

[JCook21]

I've just created PR #111 to address this. It overrides the say_status and system methods to perform checking for these options in the one place. Comments welcome as ever!