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

Mixin inheritance chain doesn't work correctly #14

Open
wasnotrice opened this issue May 23, 2012 · 6 comments
Open

Mixin inheritance chain doesn't work correctly #14

wasnotrice opened this issue May 23, 2012 · 6 comments

Comments

@wasnotrice
Copy link
Member

Consider this scenario: I am writing SwtShoes::CommonMethods. I can easily overwrite methods in Shoes::CommonMethods by defining them in my module, because the inheritance chain goes like this:

Shoes::Button -> SwtShoes::Button -> SwtShoes::CommonMethods -> Shoes::CommonMethods

Now say I want to override a method on SwtShoes::Button. If I try this in SwtShoes::Button, my methods do not override methods in Shoes::Button, because of the same inheritance chain. This is confusing, and incorrect in the sense that SwtShoes::Button ought to have the first crack at the behavior, since it is the most specific of the modules/classes.

After working with this structure for a while, I think that we should try adding framework-specific behavior to Shoes classes (e.g. Shoes::Button) through composition instead of mixins. For modules (e.g. Shoes::CommonMethods), simply mixing in a framework-specific module seems to work. Testing a module is harder, though, than testing a class, especially when the module depends on state, which lots do in Shoes.

@pjfitzgibbons
Copy link
Member

So I see two issues to correct. 1) too many levels of dependency (4). 2)
cyclic hierarchy (Shoes then SwtShoes then Shoes).

What would work for a simpler model?
Peter Fitzgibbons
(847) 859-9550
Email: peter.fitzgibbons@gmail.com
IM GTalk: peter.fitzgibbons
IM AOL: peter.fitzgibbons@gmail.com

On Wed, May 23, 2012 at 2:31 PM, Eric Watson <
reply@reply.github.com

wrote:

Consider this scenario: I am writing SwtShoes::CommonMethods. I can
easily overwrite methods in Shoes::CommonMethods by defining them in my
module, because the inheritance chain goes like this:

Shoes::Button -> SwtShoes::Button -> SwtShoes::CommonMethods ->
Shoes::CommonMethods

Now say I want to override a method on SwtShoes::Button. If I try this
in SwtShoes::Button, my methods do not override methods in
Shoes::Button, because of the same inheritance chain. This is confusing,
and incorrect in the sense that SwtShoes::Button ought to have the first
crack at the behavior, since it is the most specific of the modules/classes.

After working with this structure for a while, I think that we should try
adding framework-specific behavior to Shoes classes (e.g. Shoes::Button)
through composition instead of mixins. For modules (e.g.
Shoes::CommonMethods), simply mixing in a framework-specific module seems
to work. Testing a module is harder, though, than testing a class,
especially when the module depends on state, which lots do in Shoes.


Reply to this email directly or view it on GitHub:
#14

@wasnotrice
Copy link
Member Author

Here's one possibility: For Shoes classes, we add an instance variable that acts as the point of interaction with the backend. So instead of this:

class Shoes::Button
  def initialize(...)
    gui_init
  end
end

module SwtShoes::Button
  def gui_init
  end
end

we have this:

class Shoes::Button
  def initialize(...)
    # Create appropriate class using Shoes.configuration.framework
    @gui = Shoes.gui_for(self) # @gui.class == SwtShoes::Button
    @gui.init
  end
end

class SwtShoes::Button
  def init
  end
end

in purple_shoes, ashbb uses @real instead of @gui. I kind of like that name, too.

To increase testability of the Shoes classes, we can allow the framework class to be passed into the constructor:

class Shoes::Button
  def initialize(args={})
    @gui = args[:gui] || Shoes.gui_for(self)
    @gui.init
  end
end

The key wins are

  1. Remove one level of inheritance
  2. Gui framework code isn't coupled to Shoes classes
  3. Gui framework code is more easily tested (no Shoelaces)
  4. Shoes code doesn't contain as many method calls like gui_init, for which you have to search up the hierarchy to find where the method actually lives--you know that @gui.init is calling a method on the @gui object.
  5. The framework class is the bottom level in terms of specificity

@pjfitzgibbons
Copy link
Member

HI Eric,

Ok, what you demonstrate makes total sense, and I would have to think for a
good long time to figure how how in the heck I managed to code the current
spaghetti. Oof, sorry for that.

2 Questions :
Q: Is SwtShoes::Button a mixin to Shoes::Button? I remember some
auto-loading framework code... is that expected?
Q: This might be the ripe time to refactor the namespace. I'm wondering,
should the plugin be a namespace extension fo every base-class :
Shoes::Button::Swt ?

Looking forward to your thoughts.
Peter Fitzgibbons
(847) 859-9550
Email: peter.fitzgibbons@gmail.com
IM GTalk: peter.fitzgibbons
IM AOL: peter.fitzgibbons@gmail.com

On Wed, May 23, 2012 at 3:57 PM, Eric Watson <
reply@reply.github.com

wrote:

Here's one possibility: For Shoes classes, we add an instance variable
that acts as the point of interaction with the backend. So instead of this:

class Shoes::Button
 def initialize(...)
   gui_init
 end
end

module SwtShoes::Button
 def gui_init
 end
end

we have this:

class Shoes::Button
 def initialize(...)
   # Create appropriate class using Shoes.configuration.framework
   @gui = Shoes.gui_for(self) # @gui.class == SwtShoes::Button
   @gui.init
 end
end

class SwtShoes::Button
 def init
 end
end

in purple_shoes,
ashbb uses @real instead of @gui. I kind of like that name, too.

To increase testability of the Shoes classes, we can allow the framework
class to be passed into the constructor:

class Shoes::Button
 def initialize(args={})
   @gui = args[:gui] || Shoes.gui_for(self)
   @gui.init
 end
end

The key wins are

  1. Remove one level of inheritance
  2. Gui framework code isn't coupled to Shoes classes
  3. Gui framework code is more easily tested (no Shoelaces)
  4. Shoes code doesn't contain as many method calls like gui_init, for
    which you have to search up the hierarchy to find where the method actually
    lives--you know that @gui.init is calling a method on the @gui object.
  5. The framework class is the bottom level in terms of specificity

Reply to this email directly or view it on GitHub:
#14 (comment)

@wasnotrice
Copy link
Member Author

I had to code lots of classes before I ran into a problem with this structure, so I think it was a completely reasonable idea. We adapt.

2 Answers:

A: Yes, currently, SwtShoes::Button is a mixin to Shoes::Button, and likewise for other Shoes classes and SwtShoes modules. In the SwtShoes::Button file, there is a reopening of Shoes::Button to mixin the SwtShoes module. It is expected in the sense that everything is currently coded that way.

A: Hmmm. I think we could go either way on this. Shoes::Button::Swt makes sense because the framework buttons kind of belong to the Shoes class. But when I visualize it:

Shoes                   Shoes
  Animation               Animation
  Button                    Swt
  Flow                      Swing
  ...                        ...
  Swt                     Button
    Animation               Swt
    Button                  Swing
    Flow                    ...
    ...                   Flow
  Swing                     Swt
    Animation               Swing
    Button                  ...
    Flow                  ...
    ...
  ...

I think it makes more sense to namespace like Shoes::Swt::Button. Since the gui framework classes are going to be bundled together, it makes sense that they are namespaced together. Compare:

Shoes                   Shoes
  Swt                     Animation
    Animation               Swt
    Button                Button
    Flow                    Swt
    ...                   Flow
                            Swt
                            ...

Thoughts?

@pjfitzgibbons
Copy link
Member

Ok. I agree with your design.

So Shoes::Animation is loaded, then by framework-loader,
Shoes::Swt::Animation is loaded.

Should the GUI interface use before/after hooks to do its work? Do you
have a vision for this?

Peter Fitzgibbons
(847) 859-9550
Email: peter.fitzgibbons@gmail.com
IM GTalk: peter.fitzgibbons
IM AOL: peter.fitzgibbons@gmail.com

On Wed, May 23, 2012 at 10:20 PM, Eric Watson <
reply@reply.github.com

wrote:

I had to code lots of classes before I ran into a problem with this
structure, so I think it was a completely reasonable idea. We adapt.

2 Answers:

A: Yes, currently, SwtShoes::Button is a mixin to Shoes::Button, and
likewise for other Shoes classes and SwtShoes modules. In the
SwtShoes::Button file, there is a reopening of Shoes::Button to mixin
the SwtShoes module. It is expected in the sense that everything is
currently coded that way.

A: Hmmm. I think we could go either way on this. Shoes::Button::Swt
makes sense because the framework buttons kind of belong to the Shoes
class. But when I visualize it:

Shoes                   Shoes
 Animation               Animation
 Button                    Swt
 Flow                      Swing
 ...                        ...
 Swt                     Button
   Animation               Swt
   Button                  Swing
   Flow                    ...
   ...                   Flow
 Swing                     Swt
   Animation               Swing
   Button                  ...
   Flow                  ...
   ...
 ...

I think it makes more sense to namespace like Shoes::Swt::Button. Since
the gui framework classes are going to be bundled together, it makes sense
that they are namespaced together. Compare:

Shoes                   Shoes
 Swt                     Animation
   Animation               Swt
   Button                Button
   Flow                    Swt
   ...                   Flow
                           Swt
                           ...

Thoughts?


Reply to this email directly or view it on GitHub:
#14 (comment)

@pjfitzgibbons
Copy link
Member

HI Eric,

I created Shoes4 issue #4 : Fix mixin/inheritance chain

We can continue our discussion there.

Peter Fitzgibbons
(847) 859-9550
Email: peter.fitzgibbons@gmail.com
IM GTalk: peter.fitzgibbons
IM AOL: peter.fitzgibbons@gmail.com

On Thu, May 24, 2012 at 5:35 AM, Peter Fitzgibbons <
peter.fitzgibbons@gmail.com> wrote:

Ok. I agree with your design.

So Shoes::Animation is loaded, then by framework-loader,
Shoes::Swt::Animation is loaded.

Should the GUI interface use before/after hooks to do its work? Do you
have a vision for this?

Peter Fitzgibbons
(847) 859-9550
Email: peter.fitzgibbons@gmail.com
IM GTalk: peter.fitzgibbons
IM AOL: peter.fitzgibbons@gmail.com

On Wed, May 23, 2012 at 10:20 PM, Eric Watson <
reply@reply.github.com

wrote:

I had to code lots of classes before I ran into a problem with this
structure, so I think it was a completely reasonable idea. We adapt.

2 Answers:

A: Yes, currently, SwtShoes::Button is a mixin to Shoes::Button, and
likewise for other Shoes classes and SwtShoes modules. In the
SwtShoes::Button file, there is a reopening of Shoes::Button to mixin
the SwtShoes module. It is expected in the sense that everything is
currently coded that way.

A: Hmmm. I think we could go either way on this. Shoes::Button::Swt
makes sense because the framework buttons kind of belong to the Shoes
class. But when I visualize it:

Shoes                   Shoes
 Animation               Animation
 Button                    Swt
 Flow                      Swing
 ...                        ...
 Swt                     Button
   Animation               Swt
   Button                  Swing
   Flow                    ...
   ...                   Flow
 Swing                     Swt
   Animation               Swing
   Button                  ...
   Flow                  ...
   ...
 ...

I think it makes more sense to namespace like Shoes::Swt::Button. Since
the gui framework classes are going to be bundled together, it makes sense
that they are namespaced together. Compare:

Shoes                   Shoes
 Swt                     Animation
   Animation               Swt
   Button                Button
   Flow                    Swt
   ...                   Flow
                           Swt
                           ...

Thoughts?


Reply to this email directly or view it on GitHub:
#14 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants