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

Raise exception if dependency is already callable #15

Merged
merged 5 commits into from
Mar 16, 2020
Merged

Conversation

iovis
Copy link
Contributor

@iovis iovis commented Mar 16, 2020

The work done for #7 changed how we aliased #call by adding it to the singleton class of the instance instead of returning a lambda with the behavior (so it can accept blocks if needed).

This had some benefits, but also could introduce some unexpected behavior if the original class already responded to #call.

For instance, when using ActionCable.server (which acts as a singleton), it would shadow the #call used as a Rack server to listen to clients wanting to register a WebSocket.

This work ensures the developer doesn't introduce unexpected side effects when inverting third party library dependencies.

@iovis iovis self-assigned this Mar 16, 2020
@iovis iovis requested a review from amrocco March 16, 2020 21:42
@@ -16,6 +16,10 @@ def wrap_args(args)
def wrap_call(the_instance)
return the_instance unless call

if the_instance.respond_to? :call
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Injectable::Dependency#wrap_call manually dispatches method call

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't even know what that means

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

XD

@@ -0,0 +1,4 @@
module Injectable
class MethodAlreadyExistsException < RuntimeError
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Injectable::MethodAlreadyExistsException has no descriptive comment

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it has a very descriptive name 🎤 💧

@codeclimate
Copy link

codeclimate bot commented Mar 16, 2020

Code Climate has analyzed commit 4295b65 and detected 3 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 3

View more on Code Climate.

Copy link
Contributor

@amrocco amrocco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@iovis iovis merged commit 29d93a0 into master Mar 16, 2020
@iovis iovis deleted the david/call_shadow branch March 16, 2020 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants