Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Superclasses, Children and unexpected results (self is not what it should be) #41

Open
PragTob opened this issue Feb 17, 2020 · 12 comments
Labels
2.0 Feature request for 2.0
Milestone

Comments

@PragTob
Copy link
Contributor

PragTob commented Feb 17, 2020

Hi there!

First, thanks a lot for your work! 💚

Problematic Script

require "docile"

class Parent
  attr_reader :children

  def initialize
    @children = []
  end

  def add_child(clazz, &block)
    child = clazz.new
    children << child

    Docile.dsl_eval(child, &block) if block
    child
  end
end

class CustomParent < Parent
  def do_stuff
    add_child Parent do
      wanna_be_custom
    end
  end

  def wanna_be_custom
    add_child CustomParent
  end
end

parent = CustomParent.new
parent.do_stuff

p parent.children

Actual Output

Both Children are added to the main parent/within the block self isn't shifted to the child

[#<Parent:0x000055794759c598 @children=[]>, #<CustomParent:0x00005579475ab7a0 @children=[]>]

Expected Output

the main parent has one child, which itself then has the CustomParent as a child. (within the block self correctly is the child)

[#<Parent:0x000055d997c50460 @children=[#<CustomParent:0x000055d997c5b658 @children=[]>]>]

This output can currently be achieved in 2 ways:

  1. inline wanna_be_custom
  def do_stuff
    add_child Parent do
      add_child CustomParent
    end
  end
  1. in add_child, instead of instantiating the passed in class just use self.class (yes this changes semantics but might give a hint as to the source of this)
  def add_child(clazz, &block)
    child = self.class.new
    children << child

    Docile.dsl_eval(child, &block) if block
    child
  end

Might be related to #31 but it's marked fixed 🤷‍♂️

edit: After a good night sleep I finally realized that of course this is about self being the wrong thing so much like #31 ;)

@PragTob PragTob changed the title Superclasses, Children and unexpected results Superclasses, Children and unexpected results (self is not what it should be) Feb 18, 2020
@ms-ati
Copy link
Owner

ms-ati commented Feb 19, 2020

Hi @PragTob, thanks for such a thorough and well-researched bug report! And such an appreciative and friendly tone 😃! I'll be happy to take a look and see if I can help

@ms-ati
Copy link
Owner

ms-ati commented Feb 19, 2020

@PragTob this is tricky because it's at the intersection of a couple different behaviors of docile, isn't it?

Observing that wanna_be_custom is only defined on CustomParent, but NOT defined on Parent: what is the expected behavior of CustomParent#do_stuff:

add_child Parent do
  wanna_be_custom
end

In english, I'm reading this as:

  1. Create an instance of the class Parent, and treat it as a DSL
  2. Call wanna_be_custom from that DSL, which is NOT a method of Parent

@ms-ati
Copy link
Owner

ms-ati commented Feb 19, 2020

In this case, docile has a feature which is called "falling back to block context", which is to go back to where the block was defined, and see if the missing method was available there.

In this case, it is going back to the instance of CustomParent where do_stuff was called, and successfully finding wanna_be_custom there.

Based on my current understanding, this appears to be expected behavior, and is covered by the unit specs here: https://github.com/ms-ati/docile/blob/master/spec/docile_spec.rb#L123-L208

@ms-ati
Copy link
Owner

ms-ati commented Feb 19, 2020

The intent of the code working this way, is that it's possible to extract "helper methods" from the call site of a DSL block, if that makes sense. Are you seeing this differently -- is there a way to change the test case to more closely match a real-world use case you are seeing?

@ms-ati
Copy link
Owner

ms-ati commented Feb 19, 2020

Or is the feature request here:

Given a block context and a DSL object which BOTH implement the same method foo (in this case #add_child)
When we call a helper method that falls back to the block context (in this case #wanna_be_custom)
And the helper method itself calls the method foo which exists in both block and DSL contexts
Then we expect the helper method's call of method foo to target the DSL object rather than its current  fallback block context

Sorry, I know this can be made clearer above, we'll need to adopt some clearer terminology here. But is this where the difference in expectation stems from?

@PragTob
Copy link
Contributor Author

PragTob commented Feb 20, 2020

@ms-ati hey, thanks for the great work and all the work investigating it :)

It might be too specific of a use case to have it solved in a general library. In my specific use case what I want is that even if the method is defined only in the "parent" that it should be called but the children should still be added to the "child".

Let me look up my test case...

basically this (taken from: matestack/matestack-ui-core@ac16b3e):

          def body
            button do
              warning_text
            end
          end

          def warning_text
            plain "WARNING! WARNING!"
          end
        end

It's an HTML DSL. Within the button (child) I want to call a method defined in the parent warning_text and its children should be added to the button (child).

I think in the end your description from above may be right, as in the end both methods resolve to add_child (i.e. button is defined roughly as add_child ButtonClass, *args, &block - same thing for every other DSL).

So... that might be the feature request in there.

But no hurries/worries I have it implemented & working myself right now. It might be too specific of a use case anyhow (although I had hoped docile would magically solve it all for me 😁 ) as the DSL I'm trying to reimplement right now does have its specifics.

Anyhow, thanks a lot for your help! 💚 🎉 💃

IMG_20171225_092009_Bokeh

@ms-ati
Copy link
Owner

ms-ati commented Feb 20, 2020

Thanks @PragTob. Ok, I understand the feature request. This issue is ONLY when a helper method defined in the block's context calls an instance method which is available in both the DSL and block's contexts.

The feature request is, in these cases, to choose to call the DSL object's method from the call-site in the block's context helper method instead of the method with the same name int he block's context.

Yes?

@ms-ati
Copy link
Owner

ms-ati commented Feb 20, 2020

Also: I would like for Docile to be useful for what you are attempting to build in matestack - a powerful multi-level DSL - so I am motivated to dig into this with you, and see if Docile can be more helpful without breaking any other use cases.

@ms-ati
Copy link
Owner

ms-ati commented Feb 20, 2020

@PragTob The challenge to this feature request is that our current implementation of falling back from helper methods in the block's context, back into the DSL object, is based on instrumenting method_missing, see: https://github.com/ms-ati/docile/blob/master/lib/docile/fallback_context_proxy.rb#L51-L62

@ms-ati
Copy link
Owner

ms-ati commented Feb 20, 2020

I vaguely plan for a future Docile 2.0 to be based on never instrumenting method_missing, or in any way mutating the contexts, and instead to use the technique of purely constructing new proxy objects that are based on the methods of the DSL and block contexts. This would be easier to solve with that putative future implementation 🤷‍♂

But for now, I can noodle a bit on seeing if there's a way to make this work in the current implementation.

@PragTob
Copy link
Contributor Author

PragTob commented Feb 21, 2020

@ms-ati thanks a bunch!

The plans for Docile 2.0 sound exciting and are vaguely how I thought it already worked tbh (I didn't really dig into the code base yet, just saw the construction of some proxy objects and thought that was a very neat and cool way to go about it 👌 ).

Thanks a lot, and as always - no pressure this is all OSS/free time activity :)

@ms-ati
Copy link
Owner

ms-ati commented Jan 13, 2021

Tagging this issue as a feature request intended for Docile 2.0

@ms-ati ms-ati added the 2.0 Feature request for 2.0 label Jan 13, 2021
@ms-ati ms-ati added this to the 2.0 milestone Jan 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.0 Feature request for 2.0
Projects
None yet
Development

No branches or pull requests

2 participants