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

Add default implementation of `Middleware#close`. #1069

Merged
merged 5 commits into from Nov 13, 2019

Conversation

@ioquatix
Copy link
Contributor

ioquatix commented Nov 1, 2019

I am waiting for airplane so this is brief just to start the conversation around how this should work.

#241

Copy link
Member

technoweenie left a comment

It looks like this will go well with Faraday::RackBuilder#close and Faraday::Connection#close, so a user can call it easily from their app.

I would like some tests to go along with this:

  • Test that Middleware can implement #close
  • Test that an Adapter can implement #close
lib/faraday/middleware.rb Outdated Show resolved Hide resolved
@iMacTia

This comment has been minimized.

Copy link
Member

iMacTia commented Nov 6, 2019

Thank you @ioquatix, this is exactly the implementation I was thinking about and it totally make sense. Agree with @technoweenie that we need to check if close exists before calling it.

The only problem with this implementation is that we're assuming all middleware inherit from Faraday::Middleware. Although that's definitely the recommendation, it's not strictly enforced in v0.x and v1.x. If someone does NOT add the close method to a custom-built middleware that does not inherit from Faraday::Middleware, then the result is that the close call-chain will be silently interrupted at that middleware.

If we're happy with that, we can polish this PR, add some tests and update the documentation, but that's one of the reasons why I initially left it for v2.0 (where the plan is to make backwards-incompatible changes and force things like all middleware to inherit)

@technoweenie

This comment has been minimized.

Copy link
Member

technoweenie commented Nov 6, 2019

then the result is that the close call-chain will be silently interrupted at that middleware.

I agree, a silent fail on #close wouldn't be ideal. Another option is to raise if @app.close doesn't exist. Subclassing Faraday::Middleware or defining #close isn't required for middleware, but anyone wanting to release internal resources should know where it failed. Maybe that could be the difference between #close and #close!

@ioquatix

This comment has been minimized.

Copy link
Contributor Author

ioquatix commented Nov 6, 2019

One option would be to make it a documented requirement for 1.0, and we can make it a warning if it doesn’t exist. That way we don’t break existing code but people are aware it’s wrong.

@technoweenie

This comment has been minimized.

Copy link
Member

technoweenie commented Nov 7, 2019

I like this, as this will only come up if a developer tries to call Faraday::Connection#close, which would be a new 1.0 feature and not common Faraday usage. It should be understandable to anyone using #close that not every middleware will support it. If it helps, the functionality in Faraday could be extracted to a module, so it's a trivial fix that doesn't force anyone to disrupt a middleware class' inheritance hierarchy.

@ioquatix

This comment has been minimized.

Copy link
Contributor Author

ioquatix commented Nov 11, 2019

Okay, I think this should be okay now? It's the bare minimum required I guess. It's such a simple interface... I probably don't recommend adding a module for it unless you can roll it into other life-cycle related behaviour.

@ioquatix ioquatix force-pushed the ioquatix:patch-1 branch from d63d1a0 to 2631333 Nov 11, 2019
@ioquatix ioquatix force-pushed the ioquatix:patch-1 branch from 2631333 to 9379c71 Nov 11, 2019
@technoweenie

This comment has been minimized.

Copy link
Member

technoweenie commented Nov 11, 2019

This looks good, but how do you actually plan to use it? If this merges, I think you'll have to call something like conn.builder.app.close to run this. Are you planning on adding something like Connection#close in a future PR?

@ioquatix

This comment has been minimized.

Copy link
Contributor Author

ioquatix commented Nov 11, 2019

Oh I just assumed connection was a kind of middleware, okay ill need to expose that too.

@ioquatix

This comment has been minimized.

Copy link
Contributor Author

ioquatix commented Nov 12, 2019

I don't know how to fix:

lib/faraday/connection.rb:15:3: C: Metrics/ClassLength: Class has too many lines. [234/233]
27
  class Connection ...
28
  ^^^^^^^^^^^^^^^^
@iMacTia

This comment has been minimized.

Copy link
Member

iMacTia commented Nov 12, 2019

@ioquatix thanks for continuing to work on this!

Change Line 16 in .rubocop_todo.yml (https://github.com/lostisland/faraday/blob/master/.rubocop_todo.yml#L16) from 233 to 234, that should fix it!

Copy link
Member

olleolleolle left a comment

All feedback has been met with an implementation and a plan ahead.

I think we are ready for this!

@technoweenie technoweenie merged commit 69f3078 into lostisland:master Nov 13, 2019
6 checks passed
6 checks passed
linting
Details
build (2.3.x)
Details
build (2.4.x)
Details
build (2.5.x)
Details
build (2.6.x) build (2.6.x)
Details
codeclimate All good!
Details
@technoweenie

This comment has been minimized.

Copy link
Member

technoweenie commented Nov 13, 2019

Thanks for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.