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

DynamicNode faked/neutered Arrays are unintelligible frankenbeasts #66

Open
atz opened this issue Jul 28, 2015 · 3 comments
Open

DynamicNode faked/neutered Arrays are unintelligible frankenbeasts #66

atz opened this issue Jul 28, 2015 · 3 comments

Comments

@atz
Copy link
Contributor

atz commented Jul 28, 2015

DynamicNode tries too hard to be invisible and fails.

    expect(apo.roleMetadata.collection_manager.include?('foo@bar.com')).to be_truthy
    expect(apo.roleMetadata.collection_manager.respond_to?(:include?)).to be_truthy

The former succeeds, the latter fails.

  1) Hydrus::AdminPolicyObject should be able to create an APO object, whose APO is the Ur-APO
     Failure/Error: expect(apo.roleMetadata.collection_manager.respond_to?(:include?)).to be_truthy
       expected: truthy value
            got: false

So the object does not think it respond_to?(:include), despite the fact it just did respond to it. Cute, eh? This is on a 1.8.x release.

@atz
Copy link
Contributor Author

atz commented Jul 29, 2015

I think this is the missing logic:
https://robots.thoughtbot.com/always-define-respond-to-missing-when-overriding

We need respond_to_missing?. We cannot really replicate all the 4-part conditional logic of method_missing:

  • it would have massive cost, since mm actually includes a constructor,
  • even if we could afford it, the constructor is dependent on having arguments that respond_to_missing? does not receive.

But, we can at least say we care about the methods defined by the val object. And we don't have to say where those calls would be serviced, just that yes they are not defined, but we will respond to them... somehow or other.

@cbeer, @mjgiarlo: thoughts?

@atz
Copy link
Contributor Author

atz commented Jul 29, 2015

In practice, method_missing will respond 4 different ways:

  • ALL assignment operations are accepted/attempted as new nodes,
  • ANY operation with multiple arguments is accepted/attempted as a new node (w/ index),
  • With an auto-constructed sub DynamicNode object,
  • By handing off to val. This is the only route that will return a sensible NoMethodError.

respond_to_missing? won't have those args, so we cannot handle cases 2 and 3. But we can at least do 1 and 4.

Note: the specialized constructor methods invoked by method_missing really should be private.

@mjgiarlo
Copy link
Member

@atz I don't see any reason not to encourage you!

atz added a commit that referenced this issue Jul 29, 2015
…ssing?

Consolidated specialized constructor methods and marked intended internal
methods as private.  Throw a sensible NoMethodError when needed! The
tests checking for NoMethodError would pass... but only because of an
inscrutable error when a method was invoked on NilClass.

Tests that demonstrate the necessity of changes.
atz added a commit that referenced this issue Jul 29, 2015
…ssing?

Consolidated specialized constructor methods and marked intended internal
methods as private.  Throw a sensible NoMethodError when needed! The
tests checking for NoMethodError would pass... but only because of an
inscrutable error when a method was invoked on NilClass.

Tests that demonstrate the necessity of changes.
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