[WIP] refactor SupportsFeatureMixin to do less metaprogramming #11322

durandom · 2016-09-16T13:46:59Z

Before SupportsFeature should not be included in modules because methods get created on inclusion time.

class Vm
  include SupportsFeatureMixin
  supports :foo
end
Vm.new.supports_foo? #> true

module Operations
  extend ActiveSupport::Concern
  include SupportsFeatureMixin
end
Operations.supports_foo? #> false

class SpecialVm < Vm
  include Operations
end
# now SpecialVm.new.supports_foo? does not resolve to Vm.new.supports_foo? but Operations.supports_foo?
SpecialVm.new.supports_foo? #> false

I think part of the oddity is the creation of methods being inside included which isn't "normal" mixin behavior. I wonder if it's better to just have the methods defined directly in the SupportsFeatureMixin, then mixed in naturally, instead of dynamically creating them into the includee.

I refactored the Mixin to not generate the supports_feature? methods upon calling the DSL supports :feature methods. The reason of doing that was to leverage rubys method lookupchain and have subclasses override default behavior of base classes.

Now I store a FeatureDefinition into a class or module variable supported_feature_definitions upon calling the DSL. When asking an object supports?(:feature) I traverse the ancestors to find the first FeatureDefinition and use that to determine if the feature is supported or not.

While both solutions look similar, the first one fails when SupportsFeatureMixin is included into a Module and the DSL methods are called on the module (like in the example above). This created methods which would hide other methods (because every feature would be unsupported by default).

I think this approach makes it easier to use, because now you can use the included block and ActiveSupport::Concern to forward the DSL to the included module. But you dont have to.

Fryguy · 2016-09-16T20:02:12Z

Can you show an example of what this actually solves? ActiveSupport::Concern is already at this modules level and should alleviate your problem unless I'm misunderstanding.

Fryguy · 2016-09-16T20:04:14Z

it should not be included twice because methods would get overwritten.

Is this even possible? Again, what is this trying to solve...

[1] pry(main)> class Foo < Object; end
=> nil
[2] pry(main)> module MyMixin; end
=> nil
[3] pry(main)> Foo.include(MyMixin)
=> Foo
[4] pry(main)> Foo.ancestors
=> [Foo, MyMixin, Object, PP::ObjectMixin, Kernel, BasicObject]
[5] pry(main)> Foo.include(MyMixin)
=> Foo
[6] pry(main)> Foo.ancestors
=> [Foo, MyMixin, Object, PP::ObjectMixin, Kernel, BasicObject]
[7] pry(main)> module AnotherMixin; include MyMixin; end
=> AnotherMixin
[8] pry(main)> Foo.include(AnotherMixin)
=> Foo
[9] pry(main)> Foo.ancestors
=> [Foo, AnotherMixin, MyMixin, Object, PP::ObjectMixin, Kernel, BasicObject]

Notice it's not there twice at the bottom (line 9) nor in line 6

Fryguy · 2016-09-16T20:06:31Z

If you do need to include it, perhaps instead of blowing up, the included method can just be written kinder such that it will not call supports_not if the method already exists. Then you can "include twice" and it won't matter because it won't do anything on the second pass.

durandom · 2016-09-19T10:57:29Z

@Fryguy sorry, I should have included a more detailed description. -> updated.

But this is still failing. I think I have to look at Module.append_features to do this kind of check. I'll also add some tests to illustrate this.

@miq-bot add_label wip

Fryguy · 2016-09-19T15:33:04Z

I think part of the oddity is the creation of methods being inside included which isn't "normal" mixin behavior. I wonder if it's better to just have the methods defined directly in the SupportsFeatureMixin, then mixed in naturally, instead of dynamically creating them into the includee.

module SupportsFeatureMixin
  def supports_not; end

  included do
    QUERYABLE_FEATURES.keys.each do |feature|
      supports_not(feature)
    end
  end
end

# instead maybe

module SupportsFeatureMixin
  def supports_not(*args); end

  QUERYABLE_FEATURES.keys.each do |feature|
    supports_not(feature)
  end
end

Fryguy · 2016-09-19T15:36:26Z

Copied from Gitter to demonstrate "normal" inclusion of methods vs dynamically defining methods in the included callback.

[1] pry(main)> module MyMixin; def foo; end; end
=> :foo
[2] pry(main)> class MyClass; include MyMixin; end
=> MyClass
[3] pry(main)> MyClass.instance_method(:foo)
=> #<UnboundMethod: MyClass(MyMixin)#foo>

[4] pry(main)> module MyMixin2; def self.included(other); other.class_eval { def foo; end }; end; end
=> :included
[5] pry(main)> class MyClass2; include MyMixin2; end
=> MyClass2
[6] pry(main)> MyClass2.instance_method(:foo)
=> #<UnboundMethod: MyClass2#foo>

Might be a little hard to see but the first 3 lines are "normal" includes...notice the method lives in MyMixin. The last 3 lines are when you dynamically define a method...notice the method lives in MyClass2.

Now, if you created a method foo in MyClass it would "override" the one in MyMixin because of the class ancestry. If you create a method foo in MyClass2, it may or may not override the mixed in one depending on the order (and in fact you lose the old one and can't even call super anymore).

chrisarcand · 2016-09-19T15:39:12Z

app/models/mixins/supports_feature_mixin.rb

@@ -74,6 +74,14 @@ module SupportsFeatureMixin
  # Whenever this mixin is included we define all features as unsupported by default.
  # This way we can query for every feature
  included do
+    if self.class == Module
+      raise "SupportsFeatureMixin cannot be included into modules. You have to use 'extend ActiveSupport::Concern'"


You have to use 'extend ActiveSupport::Concern'

Huh? 😕

😄 yea, ack this reads awkward, but I couldnt come up with a better oneliner to explain this documentation

But maybe this whole PR isnt needed at all because I suck at inclusion

durandom · 2016-09-20T09:40:48Z

The first case in the original description is still true.
My confusion was because of a subtlety in include, modules and inheritance

class A
  module SubModule
    def honk
      "overwrites with A::SubModule"
    end
  end

  def honk
    "A"
  end

  include SubModule
end

p A.new.honk
# "A"
p A.ancestors
# [A, A::SubModule, Object, Kernel, BasicObject]

class B < A
  module SubModule
    def honk
      "overwrites with SubModule B::SubModule"
    end
  end

  include SubModule
end

p B.new.honk
# "overwrites with SubModule B::SubModule"
p B.ancestors
# [B, B::SubModule, A, A::SubModule, Object, Kernel, BasicObject]

Now, part of this stems from making every feature unsupported by default upon inclusion of SupportsFeatureMixin and I could fix a couple of this by not doing this, if its included in a module. Like in 1b80d29 - but, as you see in the spec, I cant remedy the class methods.

Therefore I propose raising an error, as in the previous commit 0cc8399, in case the Mixin is included into a module, which is not the intended usage.

Fryguy · 2016-09-20T14:13:35Z

So your example is overwriting like that because A::SubModule is a completely different module from B::SubModule. That is, you are no re-opening A::SubModule but instead defining a new one. Even so, I'm not sure how your example is analogous to SupportsFeatureMixin

durandom · 2016-09-20T14:25:20Z

Its analogous to our layout of modules that get included by include_concern
In this PR #11075 SupportsFeatureMixin was included in app/models/manageiq/providers/vmware/infra_manager/vm/operations/guest.rb , which made its way into app/models/manageiq/providers/vmware/infra_manager/vm.rb
What happens then I was trying to describe here where the last two expectations would fail.

So, I'd ditch my last commit to this PR and would like to have this commit 0cc8399 merged

durandom · 2016-10-07T09:28:30Z

updated OP to be clearer

durandom · 2016-10-10T08:21:48Z

@Fryguy @chrisarcand please have a look at the updated description.
I hope this is clearer now and the Mixin is easier to understand and use now

durandom · 2016-10-14T13:13:14Z

@chrisarcand could you have a look, please?

Fryguy · 2016-10-14T16:21:26Z

app/models/mixins/supports_feature_mixin.rb

+      method_name = "supports_#{feature}?"
+
+      # defines the method on the instance
+      define_method(method_name) do


I still don't think this is necessary here...you should be able to build all this into the base part of the mixin, and then they will be included automatically (and in fact, will be properly defaulted.

Fryguy · 2016-10-14T16:22:04Z

app/models/mixins/supports_feature_mixin.rb

    unsupported[feature]
  end

  # query the instance if the feature is supported or not
  def supports?(feature)
    SupportsFeatureMixin.guard_queryable_feature(feature)
-    public_send("supports_#{feature}?")


do we even need the meta methods anymore? supports?(:feature) seems perfectly fine.

Exactly my thought as well. I'd really rather just keep it to a parameterized API if possible.

yeah, thought so too.
but I wanted to do it in a follow up, to focus this PR on the groundworks

Fryguy · 2016-10-14T16:23:40Z

app/models/mixins/supports_feature_mixin.rb

-        end
-        !unsupported.key?(feature)
-      end
+    private


Wonder why rubocop is complaining about this private (perhaps you have a second one somewhere earlier)?

@jrafanie Check it out...code climate engines to the rescue :)

is rubocop complaining? these are private class methods

Oh, now I 👀 Its codeclimate bugging us

Yeah, codeclimate :)

Fryguy · 2016-10-14T16:29:33Z

When asking an object supports?(:feature) I traverse the ancestors to find the first FeatureDefinition and use that to determine if the feature is supported or not.

When I read this the first thing I think is that you shouldn't need to traverse this yourself. Ruby's super can handle that for you. I love the idea of having class-level overrides.

So, pesudo-code-like I'm thinking

class BaseClass
  supports_not :foo # default values all at the base class that setup the class-level definitions
  supports_not :bar

  def self.supports?(feature)  # Mixed in from SupportsFeatureMixin
    supports_overridden_at_this_class_level?(feature) : supports_override_value(feature) : super
  end
end

class SubClass < BaseClass
  supports :foo
end

Then calling SubClass.supports?(:foo) would check the override at that level and return true, but calling SubClass.supports?(:bar) would not see a override at that level, and then would call super. super will go up to the BaseClass where all of the supports_not are defined, and you'd get false.

durandom · 2016-10-14T16:39:35Z

When I read this the first thing I think is that you shouldn't need to traverse this yourself. Ruby's super can handle that for you. I love the idea of having class-level overrides.

I dont think I can do this :(
This would make these specs not working

The override via super was the solution before this PR, with define_method. Which leads to problems when including the Mixin into modules.

But I can try to investigate that (again) in a follow up PR

chrisarcand · 2016-10-24T18:37:58Z

@durandom Thanks so much for your patience as I've had little time to look at this until now, not to mention catching up on all the conversation already had about this.

Lots of thoughts, but a few main ones:

I agree with @Fryguy; I feel like there definitely is a way to utilize inheritance in a more native way without having to traverse and keep track of this sorts of things ourselves. But before I dig further in to that...
...I'd love to see a clearer specification of what is expected of this API and what isn't. For example, right away in your description you mention Operations.supports_foo?. The semantic here doesn't make sense IMO, and shouldn't even be valid unless I'm missing some purpose for it. (Asking a mixin if it supports something directly). Therefore I suggest...
...some spec refactoring. The specs you've written seem exhaustive and good, but are hard to follow from the standpoint of someone who's trying to derive intended behavior from the specs. I can assist and will even just do that myself now, to make sure I understand all the cases. Update: This is very minimal stuff. PR shortly.

I'd like to continue on with this and wait on merging this for a bit longer; let's definitely have some solution (this one or another) in place and merged this week for you though.

durandom · 2016-10-24T19:01:09Z

I feel like there definitely is a way to utilize inheritance in a more native way without having to traverse and keep track of this sorts of things ourselves

This is why I came up with define_instance_method in the first place. But with the methods defined in the module I don't know how this would work. There is just no super method?!

chrisarcand · 2016-10-24T19:03:17Z

But with the methods defined in the module I don't know how this would work. There is just no super method?!

I think there's some confusion here that might be clearer when I have an example for you, if it works. Forthcoming!

chrisarcand · 2016-10-26T21:37:00Z

Opened #12227 to fix the core of what's trying to be fixed here (with simple inheritance), though the spec included in this PR does not pass (expecting the class methods to be overridden via a simple include).

Fryguy · 2016-10-27T14:41:56Z

app/models/mixins/supports_feature_mixin.rb

    end
  end

  class UnknownFeatureError < StandardError; end

+  class FeatureDefinition
+    def initialize(supported: false, block: nil, unsupported_reason: nil)


You might want to just use a struct to simplify this code

yes. actually it started out as a struct, got more complicated and became a class. Now it could be a struct again 😄
But in a follow up PR I'm adding support for class level blocks, so this can get a bit more complex again.

If you dont mind, I'd leave it as a class

now features are defined internally in a FeatureDefinition class. An instance of this is added to the module or class upon using the supports and supports_not DSL.

miq-bot · 2016-10-27T20:02:23Z

Checked commits durandom/manageiq@e812f50~...349e048 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
3 files checked, 1 offense detected

app/models/mixins/supports_feature_mixin.rb

❗ - Line 139, Col 5 - Style/TrivialAccessors - Use attr_reader to define trivial reader methods.

durandom · 2016-11-10T18:46:47Z

I think i am missing something, i was searchimg on whole project for declaration of supports_migrate? and can not find it

Just got that on gitter. Another reason to remove dynamically defined methods (which I introduced, so blame me :)

miq-bot · 2016-11-11T14:17:56Z

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

durandom · 2017-03-13T08:47:14Z

@miq-bot add_label wip

I still think this is a relevant and needed refactoring

miq-bot · 2017-10-14T04:01:06Z

This pull request has been automatically closed because it has not been updated for at least 6 months.

Feel free to reopen this pull request if these changes are still valid.

Thank you for all your contributions!

miq-bot added enhancement pluggability/providers labels Sep 16, 2016

jrafanie closed this Sep 16, 2016

jrafanie reopened this Sep 16, 2016

chessbyte assigned Fryguy Sep 16, 2016

durandom changed the title ~~safeguard inclusion of SupportsFeatureMixin~~ [WIP] safeguard inclusion of SupportsFeatureMixin Sep 19, 2016

miq-bot added the wip label Sep 19, 2016

chrisarcand reviewed Sep 19, 2016

View reviewed changes

durandom force-pushed the supports_feature_guard_include branch from 1a865d2 to 0cc8399 Compare September 20, 2016 09:00

durandom changed the title ~~[WIP] safeguard inclusion of SupportsFeatureMixin~~ safeguard inclusion of SupportsFeatureMixin Sep 20, 2016

chessbyte removed the wip label Sep 30, 2016

durandom mentioned this pull request Oct 7, 2016

CRUD for OpenStack Host Aggregates #11580

Merged

durandom force-pushed the supports_feature_guard_include branch from 1b80d29 to 6c68d8f Compare October 10, 2016 07:58

durandom changed the title ~~safeguard inclusion of SupportsFeatureMixin~~ refactor SupportsFeatureMixin to do less metaprogramming Oct 10, 2016

durandom force-pushed the supports_feature_guard_include branch from 6c68d8f to 5b79a01 Compare October 10, 2016 08:19

durandom force-pushed the supports_feature_guard_include branch from 5b79a01 to e29c715 Compare October 10, 2016 18:48

durandom mentioned this pull request Oct 14, 2016

[WIP] make features conditionally supported on class #11948

Closed

chrisarcand self-assigned this Oct 14, 2016

Fryguy reviewed Oct 14, 2016

View reviewed changes

durandom mentioned this pull request Oct 27, 2016

[WIP] SupportsFeatureMixin: Don't rewrite methods on inclusion #12227

Closed

Fryguy reviewed Oct 27, 2016

View reviewed changes

refactor SupportsFeatureMixin to use less metaprogramming

e812f50

now features are defined internally in a FeatureDefinition class. An instance of this is added to the module or class upon using the supports and supports_not DSL.

durandom force-pushed the supports_feature_guard_include branch from e29c715 to e812f50 Compare October 27, 2016 15:13

changed spec to use real features

349e048

durandom mentioned this pull request Nov 11, 2016

[EUWE] add missing ActiveSupport::Concern to middle module #12585

Merged

durandom changed the title ~~refactor SupportsFeatureMixin to do less metaprogramming~~ [WIP] refactor SupportsFeatureMixin to do less metaprogramming Mar 13, 2017

miq-bot added the wip label Mar 13, 2017

durandom mentioned this pull request Jul 19, 2017

Add a virtual column for supports_block_storage? and supports_cloud_object_store_container_create? #15600

Merged

miq-bot closed this Oct 14, 2017

[WIP] refactor SupportsFeatureMixin to do less metaprogramming #11322

[WIP] refactor SupportsFeatureMixin to do less metaprogramming #11322

Conversation

durandom commented Sep 16, 2016 • edited Loading

Fryguy commented Sep 16, 2016

Fryguy commented Sep 16, 2016 • edited Loading

Fryguy commented Sep 16, 2016 • edited Loading

durandom commented Sep 19, 2016

Fryguy commented Sep 19, 2016

Fryguy commented Sep 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durandom commented Sep 20, 2016

Fryguy commented Sep 20, 2016

durandom commented Sep 20, 2016

durandom commented Oct 7, 2016

durandom commented Oct 10, 2016

durandom commented Oct 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented Oct 14, 2016 • edited Loading

durandom commented Oct 14, 2016 • edited Loading

chrisarcand commented Oct 24, 2016 • edited Loading

durandom commented Oct 24, 2016

chrisarcand commented Oct 24, 2016

chrisarcand commented Oct 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Oct 27, 2016

durandom commented Nov 10, 2016

miq-bot commented Nov 11, 2016

durandom commented Mar 13, 2017

miq-bot commented Oct 14, 2017

durandom commented Sep 16, 2016 •

edited

Loading

Fryguy commented Sep 16, 2016 •

edited

Loading

Fryguy commented Sep 16, 2016 •

edited

Loading

Fryguy commented Oct 14, 2016 •

edited

Loading

durandom commented Oct 14, 2016 •

edited

Loading

chrisarcand commented Oct 24, 2016 •

edited

Loading

chrisarcand commented Oct 26, 2016 •

edited

Loading