define_method + yield would not work. Use Proc#call instead

[amatsuda]

The prervious code tries to call the given block via yield, but ruby won't actually run this as expected.

Basically, you can't call yield in a method defined by define_method. This would cause a SyntaxError.

$ ruby -e "define_method(:a) { yield }; a { p :hello }"
Traceback (most recent call last):
	1: from -e:1:in `<main>'
-e:1:in `block in <main>': no block given (yield) (LocalJumpError)

But in this case here in colorizer.rb, we're not seeing a SyntaxError because the yield tries to call a block from an outer scope, i.e., the Module scope.
So, this code will cause another error, LocalJumpError.

$ ruby -e "
  module M
    define_method(:a) { yield }
  end

  extend M

  a { p :hello }"
Traceback (most recent call last):
	1: from -e:8:in `<main>'
-e:3:in `block in <module:M>': no block given (yield) (LocalJumpError)

In order to properly give a block via define_method, we need to explicitly pass a Proc as a parameter.

$ ruby -e "
module M
  define_method(:b) do |&block|
    block.call
  end
end

extend M

b { p :hello }"
:hello

[olleolleolle]

The description is very clear and explains how the previous code's block_given? is about the scope where define_method is invoked.

Thanks a lot for adding this!

Let's get this merged! 🎉

[luke-hill]

I've used this in my own work so I know this is correct.

However, the only question is does this need adding to the 0.14.x series @mvz

[mvz]

I'll take a look at this tomorrow.

[mvz]

Hm .. okay that was a deep rabbit hole. Here's what I found:

• This code is only used to color one string cyan, so the yield bit is never actually used and in fact most of this module is unused.
• This code was copied from Cucumber
• Cucumber copied the code from term-ansicolor because that gem used the Term namespace which caused problems with applications using Term for themselves.
• The pull request that introduced the code into Cucumber (Remove dependency term-ansicolor cucumber-ruby#43) mentions that term-ansicolor was GPL'd at the time so it shouldn't have been depended on. But then why was that pull request merged?

My proposal would be:

• Step 1: Remove this code from Aruba and perhaps provide a small stub to render text in the color cyan.
• Step 2: Hook into the underlying testing framework's coloring logic so that e.g. RSpec's --color and --no-color arguments work.

[luke-hill]

Given the age of some of this codebase, I'm for anything that removes stuff, because the majority of people originally committing it are unlikely to remember why/what it was for.

Also delegating to RSpec's --color logic makes sense given this is reasonably well maintained.

[olleolleolle]

Also: fantastic sleuthing!

[amatsuda]

Closing this because the target code has been completely replaced via #636 🎉