Skip to content

Return on guard #157

Merged
merged 10 commits into from Oct 11, 2011

4 participants

@earlonrails

These are the changes that I made on this other branch. Which uses the guard options to allow for any object to be the return.

@netzpirat

I haven't found a reference to a @optional_return tag and Anything isn't a valid type either. I think you should drop the line and modify the normal return statement:

# @return [Array<Object>] the matched watcher response
@netzpirat

In idiomatic Ruby you won't write the return statement explicit, instead you leave a blank line before to clearly separate the return statement visually.

@netzpirat

Indention is wrong here.

@netzpirat

Is this desired behavior? I think this should be filtered in the Watcher.

@netzpirat

You should keep the description in sync with the code. This applies to more places than this, but here is a good example where the description says it does not return something but the spec tests a return value.

@netzpirat

As @rymai already commented, this removes some regression specs of the wide spread watcher usage: Matching files as Strings. Since this is the most important usage of Guard, this shouldn't be changed. I suggest that you leave the original specs that validates file matching as String and surround these in a context block and add your watcher specs that returns Objects into another context block.

context 'for a Watcher that matches file strings` do
  # Original specs
end

context 'for a Watcher that matches information objects' do
  # Your specs goes here
end
earlonrails added some commits Oct 7, 2011
@earlonrails earlonrails watcher init now has an addition param that allows the user to return…
… any obj type. The spec has been restored to its original and then more specs added for object support.
33542ac
@earlonrails earlonrails updated the watch spec to describe the tests better. a7b6ebe
@netzpirat

You saved three characters in any_rtn and everybody looking at this in half a year needs three seconds more until he get the point.

@netzpirat

Always add new parameters to the documentation.

@netzpirat

Here you still would know what wa means, if you understand any_rtn.

@netzpirat

Was it a watcher action or a watcher array? Ah. watch anything? You can simply check if something is idiomatic Ruby or not: Try to read it aloud.

@netzpirat

Please don't take these comments personal. They just reflect my experience from working on very large codebases. They all start to rot with stuff that seems unimportant, but when you've them over and over your code, you'll start to have a serious problem.

Guard member

I totally agree with you, I'd have had the same comments. :)

@rymai
Guard member

Please add a blank commented line here (before the method signature). :)

@rymai
Guard member

Please add a space after { and before } here.

@rymai
Guard member

Space after {!

@rymai
Guard member

You should use do...end here since you have two lines in your block.

@rymai
Guard member

BTW, it now feels wrong to me to call the accumulator paths when it can now be populated by any objects.

@rymai
Guard member

You can safely remove the space after !. Now it feels like the poor ! is lost between dad && and mom result.empty? but is really belonging to mom here! :D

Btw, this entire if...elsif...end can probably be optimized, but don't bother with that now, we'll see what can be done later.

LOL. Where's mom?

@rymai
Guard member

You should just use options for that, so there would be no changes here. Just a new documentation item above maybe.

How is this ?

attr_accessor :watchers, :options, :group, :any_return

# Initialize a Guard.
#
# @param [Array<Guard::Watcher>] watchers the Guard file watchers
# @param [Hash] options the custom Guard options
#
def initialize(watchers = [], options = {})
  @group = options[:group] ? options.delete(:group).to_sym : :default
  @watchers, @options, @any_return = watchers, options, options[:any_return]
end

** Nevermind I figured it out
You don't need attr_accessor for it cause we already have one for options.

Guard member

Exactly! The last version in the pull-request is perfect (except for the useless accessor in Watcher).

@rymai
Guard member

And use guard.options[:any_return] here (and just below).

@rymai
Guard member
rymai commented on bdfdf45 Oct 10, 2011

Other than my 2 comments, well done for all the little fix we pointed out before! :)

@rymai rymai commented on an outdated diff Oct 10, 2011
lib/guard/watcher.rb
@@ -6,7 +6,7 @@ module Guard
#
class Watcher
- attr_accessor :pattern, :action
+ attr_accessor :pattern, :action, :any_return
@rymai
Guard member
rymai added a note Oct 10, 2011

I think you can remove the last accessor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rymai
Guard member
rymai commented Oct 10, 2011

Ok, excluding my last comment, everything looks good to me! Well done!

@rymai rymai commented on an outdated diff Oct 10, 2011
lib/guard/watcher.rb
else
paths << matches[0]
end
end
end
-
- paths.flatten.map { |p| p.to_s }
+
+ guard.options[:any_return] ? paths : paths.flatten.map{ |p| p.to_s }
@rymai
Guard member
rymai added a note Oct 10, 2011

One space missing at the beginning of the line here! :P (YES, I SAW THIS! ^^)

Edit: Oh, and please add a space between map and { too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rymai
Guard member
rymai commented Oct 10, 2011

Again sorry for the comments one the coding-style details! ^^ If you're already too annoyed, don't bother and we'll do it ourselves! :P

@earlonrails

No problem Rymai. Thanks for helping me become a better programmer :D

@earlonrails

Also thnx to netzpirat too !

@netzpirat

Looks fine. I'll merge it now.

@netzpirat netzpirat merged commit 5429d10 into guard:master Oct 11, 2011
@netzpirat

Merge went fine, I just added some more documentation and split two context blocks in the watcher specs apart. The golden rule with context is, that only one assumption should go into a context description. If there are two, split and nest them.

Thanks for your work, the implementation finally looks very nice!

@rymai
Guard member
rymai commented Oct 11, 2011

Thanks all!

@thibaudgg
Guard member

Thanks all, that's awesome!

@earlonrails

Thanks all :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.