Remove Cask::Decorator

[federicobond]

@rolandwalker this is the piece of code I was talking about in IRC.

[rolandwalker]

@federicobond, it is indeed a fun bug. For those who weren't on IRC, there is an unexpected namespace collision between a Cask base.rb and class Cask::DSL::Base created in this PR. A simple way to exercise the problem is to run brew cask info base having checked out this branch.

The reason that Cask.const_get(cask_class_name) returns Cask::DSL::Base for base.rb (instead of the expected Base) is that class Cask extends Cask::DSL, though we use the included trick to implement that instead of the more explicit class Cask; extend Cask::DSL; .... (I'm not enough of a Rubyist to know why the included idiom is better.)

So, Base is a constant in class Cask. We defined that constant as Cask::DSL::Base. Therefore Ruby is giving the correct result on Cask.const_get. We could encounter the same bug in other ways, and it is maybe surprising that we didn't before. I also could not create a Cask called tags.rb.

Interestingly, the bug is affected by #5365, though not exactly caused by it. I don't have the minimal fix worked out, but both

• reverting forward-compatibility to remove naming limitations on Casks #5365 and
• changing Cask.const_get(cask_class_name).new to Kernel.const_get(cask_class_name).new

is sufficient to run brew cask info base on this branch.

Kernel.const_get seems like a reasonable change, though we should find out what is most idiomatic (@phinze?). Presumably, we can keep #5365, but modify the second parameter of the eval (currently TOPLEVEL_BINDING). The eval is OK, see below.

(The second parameter to const_get looked promising, but is apparently not what we need here.)

[rolandwalker]

Aha! Reverting #5365 is not needed. That was only helpful because that code also uses Cask.const_get. Changing both usages to Kernel.const_get is sufficient.

[phinze]

Hey peeps - @rolandwalker filled me in here. What an interesting bit of Ruby OO arcana!

After getting caught up, I agree that we should change the Cask.const_get line. I'm pretty sure we were always falling back to the inherit behavior mentioned in the docs for the happy path.

const_get(sym, inherit=true)
Checks for a constant with the given name in mod If inherit is set, the lookup will also search the ancestors (and Object if mod is a Module.)

http://www.ruby-doc.org/core-1.9.3/Module.html#method-i-const_get

I think it might be a little more idiomatic to use Object.const_get instead of Kernel.const_get. Even though both will work, Kernel is a only a module that gets mixed in to Object, whereas Object is more of a proper root of the Ruby object hierarchy.

Here's an example to illustrate:

# Simple class in top-level namespace
class Booger; end

# With const_get's inherit parameter at it's default of true
Kernel.const_get('Booger') # => Booger
Object.const_get('Booger') # => Booger
Booger.const_get('Booger') # => Booger

# Setting it to false illustrates that Object is really where the constant lies
Kernel.const_get('Booger', false) # => NameError: uninitialized constant Kernel::Booger
Object.const_get('Booger', false) # => Booger
Booger.const_get('Booger', false) # => NameError: uninitialized constant Booger::Booger

You get similar results by examining the .constants list of each context.

Great work digging into this, folks! 🚧 👷

[federicobond]

That's great! Thanks for all the helpful answers. I'll submit another pull request with just the fix for the loading behavior, which is a different issue from removing the decorator.

[federicobond]

Branch is mergeable now. I think this is a better approach to the mini-dsl problem and more consistent with the rest of the codebase.

[rolandwalker]

Great, merging. We need to remember to track the new functionality as part of DSL 1.1.