Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Return on guard #157

Merged
merged 10 commits into from

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
Owner

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
@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
Owner

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.

Owner

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

@rymai

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

@rymai

Please add a space after { and before } here.

@rymai

Space after {!

@rymai

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

@rymai

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

@rymai

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

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.

Owner

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

@rymai

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

@rymai
Owner

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

lib/guard/watcher.rb
@@ -6,7 +6,7 @@ module Guard
#
class Watcher
- attr_accessor :pattern, :action
+ attr_accessor :pattern, :action, :any_return
@rymai Owner
rymai added a note

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
Owner

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

lib/guard/watcher.rb
((10 lines not shown))
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 Owner
rymai added a note

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
Owner

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
Owner

Looks fine. I'll merge it now.

@netzpirat netzpirat merged commit 5429d10 into guard:master
@netzpirat
Owner

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
Owner

Thanks all!

@thibaudgg
Owner

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
Commits on Oct 5, 2011
  1. @earlonrails
Commits on Oct 6, 2011
  1. @earlonrails

    any return is ok. Pass the path and some other parameters if you woul…

    earlonrails authored
    …d like and they don't become strings!
  2. @earlonrails
Commits on Oct 7, 2011
  1. @earlonrails

    watcher init now has an addition param that allows the user to return…

    earlonrails authored
    … any obj type. The spec has been restored to its original and then more specs added for object support.
  2. @earlonrails
Commits on Oct 9, 2011
  1. @earlonrails

    watcher cleaned up.

    earlonrails authored
  2. @earlonrails

    fixed spec for guard

    earlonrails authored
Commits on Oct 10, 2011
  1. @earlonrails
  2. @earlonrails
  3. @earlonrails
This page is out of date. Refresh to see the latest.
Showing with 118 additions and 34 deletions.
  1. +8 −4 lib/guard/watcher.rb
  2. +110 −30 spec/guard/watcher_spec.rb
View
12 lib/guard/watcher.rb
@@ -38,7 +38,7 @@ def initialize(pattern, action = nil)
#
# @param [Guard::Guard] guard the guard which watchers are used
# @param [Array<String>] files the changed files
- # @return [Array<String>] the matched files
+ # @return [Array<Object>] the matched watcher response
#
def self.match_files(guard, files)
guard.watchers.inject([]) do |paths, watcher|
@@ -46,14 +46,18 @@ def self.match_files(guard, files)
if matches = watcher.match_file?(file)
if watcher.action
result = watcher.call_action(matches)
- paths << Array(result) if result.respond_to?(:empty?) && !result.empty?
+ if guard.options[:any_return]
+ paths << result
+ elsif result.respond_to?(:empty?) && !result.empty?
+ paths << Array(result)
+ end
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 }
end
end
View
140 spec/guard/watcher_spec.rb
@@ -46,7 +46,11 @@
end
describe ".match_files" do
- before(:all) { @guard = Guard::Guard.new }
+ before(:all) do
+ @guard = Guard::Guard.new
+ @guard_any_return = Guard::Guard.new
+ @guard_any_return.options[:any_return] = true
+ end
context "with a watcher without action" do
context "that is a regex pattern" do
@@ -65,8 +69,8 @@
end
end
end
-
- context "with a watcher action without parameter" do
+
+ context "with a watcher action without parameter for a watcher that matches file strings" do
before(:all) do
@guard.watchers = [
described_class.new('spec_helper.rb', lambda { 'spec' }),
@@ -103,56 +107,132 @@
end
end
- context "with a watcher action that takes a parameter" do
+ context 'with a watcher action without parameter for a watcher that matches information objects' do
before(:all) do
- @guard.watchers = [
+ @guard_any_return.watchers = [
+ described_class.new('spec_helper.rb', lambda { 'spec' }),
+ described_class.new('addition.rb', lambda { 1 + 1 }),
+ described_class.new('hash.rb', lambda { Hash[:foo, 'bar'] }),
+ described_class.new('array.rb', lambda { ['foo', 'bar'] }),
+ described_class.new('blank.rb', lambda { '' }),
+ described_class.new(/^uptime\.rb/, lambda { `uptime > /dev/null` })
+ ]
+ end
+
+ it "returns a single file specified within the action" do
+ described_class.match_files(@guard_any_return, ['spec_helper.rb']).class.should == Array
+ described_class.match_files(@guard_any_return, ['spec_helper.rb']).empty?.should == false
+ end
+
+ it "returns multiple files specified within the action" do
+ described_class.match_files(@guard_any_return, ['hash.rb']).should == [{:foo => 'bar'}]
+ end
+
+ it "returns multiple files by combining the results of different actions" do
+ described_class.match_files(@guard_any_return, ['spec_helper.rb', 'array.rb']).should == ['spec', ['foo', 'bar']]
+ end
+
+ it "returns the evaluated addition argument in an array" do
+ described_class.match_files(@guard_any_return, ['addition.rb']).class.should == Array
+ described_class.match_files(@guard_any_return, ['addition.rb'])[0].should == 2
+ end
+
+ it "returns nothing if the action response is empty string" do
+ described_class.match_files(@guard_any_return, ['blank.rb']).should == ['']
+ end
+
+ it "returns nothing if the action returns empty string" do
+ described_class.match_files(@guard_any_return, ['uptime.rb']).should == ['']
+ end
+ end
+
+ context "with a watcher action that takes a parameter for a watcher that matches file strings" do
+ before(:all) do
+ @guard.watchers = [
+ described_class.new(%r{lib/(.*)\.rb}, lambda { |m| "spec/#{m[1]}_spec.rb" }),
+ described_class.new(/addition(.*)\.rb/, lambda { |m| 1 + 1 }),
+ described_class.new('hash.rb', lambda { |m| Hash[:foo, 'bar'] }),
+ described_class.new(/array(.*)\.rb/, lambda { |m| ['foo', 'bar'] }),
+ described_class.new(/blank(.*)\.rb/, lambda { |m| '' }),
+ described_class.new(/uptime(.*)\.rb/, lambda { |m| `uptime > /dev/null` })
+ ]
+ end
+
+ it "returns a substituted single file specified within the action" do
+ described_class.match_files(@guard, ['lib/my_wonderful_lib.rb']).should == ['spec/my_wonderful_lib_spec.rb']
+ end
+
+ it "returns multiple files specified within the action" do
+ described_class.match_files(@guard, ['hash.rb']).should == ['foo', 'bar']
+ end
+
+ it "returns multiple files by combining the results of different actions" do
+ described_class.match_files(@guard, ['lib/my_wonderful_lib.rb', 'array.rb']).should == ['spec/my_wonderful_lib_spec.rb', 'foo', 'bar']
+ end
+
+ it "returns nothing if the action returns something other than a string or an array of strings" do
+ described_class.match_files(@guard, ['addition.rb']).should == []
+ end
+
+ it "returns nothing if the action response is empty" do
+ described_class.match_files(@guard, ['blank.rb']).should == []
+ end
+
+ it "returns nothing if the action returns nothing" do
+ described_class.match_files(@guard, ['uptime.rb']).should == []
+ end
+ end
+
+ context "with a watcher action that takes a parameter for a watcher that matches information objects" do
+ before(:all) do
+ @guard_any_return.watchers = [
described_class.new(%r{lib/(.*)\.rb}, lambda { |m| "spec/#{m[1]}_spec.rb" }),
- described_class.new(/addition(.*)\.rb/, lambda { |m| 1 + 1 }),
- described_class.new('hash.rb', lambda { Hash[:foo, 'bar'] }),
- described_class.new(/array(.*)\.rb/, lambda { |m| ['foo', 'bar'] }),
+ described_class.new(/addition(.*)\.rb/, lambda { |m| (1 + 1).to_s + m[0] }),
+ described_class.new('hash.rb', lambda { |m| Hash[:foo, 'bar', :file_name, m[0]] }),
+ described_class.new(/array(.*)\.rb/, lambda { |m| ['foo', 'bar', m[0]] }),
described_class.new(/blank(.*)\.rb/, lambda { |m| '' }),
described_class.new(/uptime(.*)\.rb/, lambda { |m| `uptime > /dev/null` })
]
end
it "returns a substituted single file specified within the action" do
- described_class.match_files(@guard, ['lib/my_wonderful_lib.rb']).should == ['spec/my_wonderful_lib_spec.rb']
+ described_class.match_files(@guard_any_return, ['lib/my_wonderful_lib.rb']).should == ['spec/my_wonderful_lib_spec.rb']
end
- it "returns multiple files specified within the action" do
- described_class.match_files(@guard, ['hash.rb']).should == ['foo', 'bar']
+ it "returns a hash specified within the action" do
+ described_class.match_files(@guard_any_return, ['hash.rb']).should == [{:foo => 'bar', :file_name => 'hash.rb'}]
end
it "returns multiple files by combining the results of different actions" do
- described_class.match_files(@guard, ['lib/my_wonderful_lib.rb', 'array.rb']).should == ['spec/my_wonderful_lib_spec.rb', 'foo', 'bar']
+ described_class.match_files(@guard_any_return, ['lib/my_wonderful_lib.rb', 'array.rb']).should == ['spec/my_wonderful_lib_spec.rb', ['foo', 'bar', "array.rb"]]
end
- it "returns nothing if the action returns something other than a string or an array of strings" do
- described_class.match_files(@guard, ['addition.rb']).should == []
+ it "returns the evaluated addition argument + the path" do
+ described_class.match_files(@guard_any_return, ['addition.rb']).should == ["2addition.rb"]
end
- it "returns nothing if the action response is empty" do
- described_class.match_files(@guard, ['blank.rb']).should == []
+ it "returns nothing if the action response is empty string" do
+ described_class.match_files(@guard_any_return, ['blank.rb']).should == ['']
end
- it "returns nothing if the action returns nothing" do
- described_class.match_files(@guard, ['uptime.rb']).should == []
+ it "returns nothing if the action returns empty string" do
+ described_class.match_files(@guard_any_return, ['uptime.rb']).should == ['']
end
end
context "with an exception that is raised" do
- before(:all) { @guard.watchers = [described_class.new('evil.rb', lambda { raise "EVIL" })] }
-
- it "displays the error and backtrace" do
- Guard::UI.should_receive(:error) { |msg|
- msg.should include("Problem with watch action!")
- msg.should include("EVIL")
- }
-
- described_class.match_files(@guard, ['evil.rb'])
- end
- end
- end
+ before(:all) { @guard.watchers = [described_class.new('evil.rb', lambda { raise "EVIL" })] }
+
+ it "displays the error and backtrace" do
+ Guard::UI.should_receive(:error) do |msg|
+ msg.should include("Problem with watch action!")
+ msg.should include("EVIL")
+ end
+
+ described_class.match_files(@guard, ['evil.rb'])
+ end
+ end
+ end
describe ".match_files?" do
before(:all) do
Something went wrong with that request. Please try again.