Edits to keep code DRY

[JCook21]

• Overrode say_status and system methods to put checks for options[:quiet] and options[:pretend] in one place.
• Edits to make sure that shell.say_status is not called since this does not invoke the overridden say_status method. Edits to tests as a result of this too.
• Removed all uses of 'unless options[:pretend]' and 'unless options[:quiet]' since this is now handled in the overridden methods.

[technicalpickles]

Should this try to extract options out of *args too? That would allow one-off programmatically quiet command, versus globally with --quiet. Same idea applies to system below.

[technicalpickles]

Definitely cleans things up nicely 👍

[JCook21]

@technicalpickles I've added some code to allow a final boolean argument to be passed to both say_status and system. I'm not sure if there's a better way to implement this than I've done though. I'd appreciate any comments.

[nickserv]

I'm thinking that say_status and the system methods could be refactored a tiny bit like this:

def boolean? object # it might be best to make this method private or protected
  object == true || object == false
end

def say_status(*args)
  quiet = args.count == 4 && boolean?(args.last) && args.pop
  super(*args) unless options[:quiet] || quiet
end

def system(*args)
  pretend = boolean?(args.last) && args.pop
  super(*args) unless options[:pretend] || pretend
end

Also, it would probably help to add some brief documentation comments for these methods either way. What do you think?

[JCook21]

@nicolasmccurdy I like that, it's neater than what I came up with. I'll add a commit with that.

[nickserv]

Looks good. 👍

[nickserv]

I just realized that due to #74 (and especially #101, which has already been merged), multiple methods would be used to avoid shell calls instead of simply calling system. As a result, it would be trickier to keep things DRY by overriding these methods while still migrating away from shell calls.

I'm thinking about these options:

1. Use some metaprogramming magic to automatically wrap methods like system, FileUtils.*, and say_status in stub commands that only perform the appropriate action if the current command line options and overridden options (if any) allow it.
2. Provide helper methods like pretend? and quiet? that look at command line and other options for you and return a boolean. This would be somewhat similar to how things worked before this pull request.
3. Require that code like unless options[:quiet] be used manually for methods besides system and say_status. This might be the simplest option, since different people may write multiple new methods that need to all use quiet and/or pretend options.

[JCook21]

@technicalpickles @nicolasmccurdy I've just had another stab at this, using meta-programming to dynamically define methods that should be 'pretendable' or 'quietable'. A couple of points about this:

• I'm not totally sure that this will work as written. In the block that used to define the method I have to access the number of parameters that the super method accepts through method(method_name).parameters.count. Do either of you know if this will work as intended?
• This will only work as written for methods within the context of the Homesick class. Do either of you have any ideas as to how this could be updated to support methods from other classes (such as FileUtils)? I was thinking of using a hash of classname => method_name. Do you think that would be possible?

[JCook21]

I've just realised that method(method_name).parameters.count doesn't seem to work. What I was trying to find are cases where methods have an extra argument and the final argument is a boolean. This falls down on methods like system though as it takes a variable number of arguments... Does anyone have any ideas on how to achieve this cleanly?

[JCook21]

This really isn't working as intended. The way I'm trying to pass a boolean value as the last argument to allow the 'quietable' or 'pretendable' behavior is not working at all. I'll take another look at what we need to achieve and how I'm trying to do it and will reopen this PR then.

[JCook21]

I've given this some more thought have decided to reopen this. The approach I was trying was to try to detect if a final boolean argument was being passed and to use that to set the pretendable or quietable behaviour. I don't think this is going to work due to the fact that many methods can have an indeterminate number of arguments (e.g. system). I think it may be better to keep this simple and just look in the options to see if the pretendable or quietable behaviour should be invoked. This could also be done programmatically by manually setting the option before calling the method that's being overridden, giving us the behaviour that @technicalpickles suggested. Let me know if you can think of a better way to achieve this.

[nickserv]

👍 Looks good to me.

By the way, you might want to merge/rebase on master now that #110 is merged.

[JCook21]

I just rebased on master and I think I've fixed the issues that came about as a result of this. @nicolasmccurdy could you have another look now?

[JCook21]

Any more comments on this or should I go ahead and merge it?

[nickserv]

Merge it!

[JCook21]

Merged!