From 2511d56aa368fda2de3575e00ffed10ea68593fe Mon Sep 17 00:00:00 2001 From: Josh Cheek Date: Mon, 28 Nov 2016 09:48:50 -0600 Subject: [PATCH 1/3] Actions adore keyword arguments Consider this task: ```ruby task :t, [:arg] do |task, arg: 'default_value'| emotion = "\e[32m^_^\e[0m" emotion = "\e[31mt.t\e[0m" if ARGV[0] =~ /t\[(.*)\]$/ && $1 != arg puts "task #{task.name.inspect} got #{arg.inspect} #{emotion}" end ``` Run this on your machine right now, you'll see the task is in tears. But this commit turns tearful eyes into smiling ones, observe: ```sh $ rake t task "t" got "default_value" ^_^ $ rake t[custom_value] task "t" got "default_value" t.t $ ruby -Ilib exe/rake t task "t" got "default_value" ^_^ $ ruby -Ilib exe/rake t[custom_value] task "t" got "custom_value" ^_^ ``` It accomplishes this by always invoking the action in the same way. Previously, Rake invoke the action with different argument structures based the arity https://github.com/ruby/rake/blob/59856100815b841624269f817c815f54921bf321/lib/rake/task.rb#L251-L256 ```ruby case act.arity when 1 act.call(self) else act.call(self, args) end ``` I assume it's expecting one of these: ```ruby proc { |task| }.arity # => 1 proc { |task, options| }.arity # => 2 ``` However this leads numerous task signatures to be invoked incorrectly. Eg, consider the case of our tearful task above, here is why it cries. ```ruby proc { |task, arg: 'default_value'| }.arity # => 1 ``` The solution is nice and easy: always invoke the block the same way. Why was this ever a thing at all, then? Looks like it was added in 2007 about a month after Ruby 1.8.6 was released. https://github.com/ruby/rake/commit/925fbb80e890029ac9310bc60270890699efc073 Probably something was just different back then, eg the difference between lambdas and procs was whether you did `prc.call` or `prc.yield` https://github.com/ruby/ruby/blob/9b383bd/eval.c#L8356-L8415 --- lib/rake/task.rb | 9 +-------- test/test_rake_task_with_arguments.rb | 25 +++++++++++++++++++++++++ 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/lib/rake/task.rb b/lib/rake/task.rb index 18c09eefd..23faf96e5 100644 --- a/lib/rake/task.rb +++ b/lib/rake/task.rb @@ -247,14 +247,7 @@ def execute(args=nil) end application.trace "** Execute #{name}" if application.options.trace application.enhance_with_matching_rule(name) if @actions.empty? - @actions.each do |act| - case act.arity - when 1 - act.call(self) - else - act.call(self, args) - end - end + @actions.each { |act| act.call(self, args) } end # Is this task needed? diff --git a/test/test_rake_task_with_arguments.rb b/test/test_rake_task_with_arguments.rb index ba28d11f9..33ec2ca08 100644 --- a/test/test_rake_task_with_arguments.rb +++ b/test/test_rake_task_with_arguments.rb @@ -80,6 +80,31 @@ def test_actions_of_various_arity_are_ok_with_args assert_equal [:a, :b, :c, :d], notes end + + def test_actions_adore_keywords + notes = [] + t = task :t, [:reqr, :ovrd, :dflt] # required, overridden-optional, default-optional + verify = lambda do |name, expecteds, actuals| + notes << name + assert_equal expecteds.length, actuals.length + expecteds.zip(actuals) { |e, a| assert_equal e, a, "(TEST #{name})" } + end + + t.enhance { |dflt: 'd', **| verify.call :a, ['d'], [dflt] } + t.enhance { |ovrd: '-', **| verify.call :b, ['o'], [ovrd] } + t.enhance { |reqr: , **| verify.call :c, ['r'], [reqr] } + + t.enhance { |t2, dflt: 'd', **| verify.call :d, [t,'d'], [t2,dflt] } + t.enhance { |t2, ovrd: 'd', **| verify.call :e, [t,'o'], [t2,ovrd] } + t.enhance { |t2, reqr: , **| verify.call :f, [t,'r'], [t2,reqr] } + + t.enhance { |t2, dflt: 'd', reqr:, **| verify.call :g, [t,'d','r'], [t2,dflt,reqr] } + t.enhance { |t2, ovrd: '-', reqr:, **| verify.call :h, [t,'o','r'], [t2,ovrd,reqr] } + + t.invoke('r', 'o') + assert_equal [*:a..:h], notes + end + def test_arguments_are_passed_to_block t = task(:t, :a, :b) { |tt, args| assert_equal({ a: 1, b: 2 }, args.to_hash) From 57869cd4e3c674a58d2045c14c6e1d202f904110 Mon Sep 17 00:00:00 2001 From: Josh Cheek Date: Mon, 28 Nov 2016 17:38:40 -0600 Subject: [PATCH 2/3] Skip test of Ruby 2.1+ features when run in older environments https://ci.appveyor.com/project/ruby/rake/build/1.0.301 --- test/test_rake_task_with_arguments.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/test_rake_task_with_arguments.rb b/test/test_rake_task_with_arguments.rb index 33ec2ca08..61f63c121 100644 --- a/test/test_rake_task_with_arguments.rb +++ b/test/test_rake_task_with_arguments.rb @@ -82,6 +82,10 @@ def test_actions_of_various_arity_are_ok_with_args def test_actions_adore_keywords + # A brutish trick to avoid parsing. Remove it once support for 1.9 and 2.0 is dropped + # https://ci.appveyor.com/project/ruby/rake/build/1.0.301 + skip 'Keywords aren\'t a feature in this version' if RUBY_VERSION =~ /^1|^2\.0/ + eval <<-RUBY, binding, __FILE__, __LINE__ notes = [] t = task :t, [:reqr, :ovrd, :dflt] # required, overridden-optional, default-optional verify = lambda do |name, expecteds, actuals| @@ -103,6 +107,7 @@ def test_actions_adore_keywords t.invoke('r', 'o') assert_equal [*:a..:h], notes + RUBY end def test_arguments_are_passed_to_block From 7d664a02ca210fb7c73f99b79a67e833bb0a6457 Mon Sep 17 00:00:00 2001 From: Josh Cheek Date: Mon, 28 Nov 2016 23:32:30 -0600 Subject: [PATCH 3/3] Fix test per https://github.com/ruby/rake/pull/174#discussion_r89925443 --- test/test_rake_task_with_arguments.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/test_rake_task_with_arguments.rb b/test/test_rake_task_with_arguments.rb index 61f63c121..9c396d3b2 100644 --- a/test/test_rake_task_with_arguments.rb +++ b/test/test_rake_task_with_arguments.rb @@ -85,13 +85,13 @@ def test_actions_adore_keywords # A brutish trick to avoid parsing. Remove it once support for 1.9 and 2.0 is dropped # https://ci.appveyor.com/project/ruby/rake/build/1.0.301 skip 'Keywords aren\'t a feature in this version' if RUBY_VERSION =~ /^1|^2\.0/ - eval <<-RUBY, binding, __FILE__, __LINE__ + eval <<-RUBY, binding, __FILE__, __LINE__+1 notes = [] t = task :t, [:reqr, :ovrd, :dflt] # required, overridden-optional, default-optional verify = lambda do |name, expecteds, actuals| notes << name assert_equal expecteds.length, actuals.length - expecteds.zip(actuals) { |e, a| assert_equal e, a, "(TEST #{name})" } + expecteds.zip(actuals) { |e, a| assert_equal e, a, "(TEST \#{name})" } end t.enhance { |dflt: 'd', **| verify.call :a, ['d'], [dflt] }