Expose pool_size as a option for the NetHttpPersistent adapter

[col]

Expose pool_size as a connection option for the NetHttpPersistent adapter

Fixes: #833

Description

• Note: connection pool was only added to net-http-persistent from version 3.0
• The pool_size constructor param in Net::HTTP::Persistent has a default value so I've been careful to ensure this will still use the default behaviour when the pool_size adapter option is not provided.

Todos

List any remaining work that needs to be done, i.e:

• Tests
• Documentation

Additional Notes

...

[iMacTia]

Good test coverage, however I believe the implementation can be improved and simplified a bit.
Please see my comment below and let me know your thoughts.

[iMacTia]

The above can be rewritten as:

options = {name: 'Faraday'}.merge(@connection_options.slice(:name, :pool_size)
Net::HTTP::Persistent.new(options)

This way we:

• Simplify the filtering using slice
• Allow to override the connection name
• Fix the issue with Ruby 1.9.3 which supports named parameters but doesn't support the double split operator (**)

proxy would also be a valid key, but I'd not slice it in as Faraday manages the proxy separately.

[col]

I believe slice is only available in rails.

I've updated the the PR but only with a small improvement to fix the 1.9.3 issue.
Out of time now but feel free to refactor if you have any other ideas.

[iMacTia]

I tried it in the console to be sure and it worked, but I just found out that's because it was also added to Ruby 2.5 🤦‍♂️.
You're right, it's not available in Ruby 2.4 or before unless using activesupport.
The new implementation doesn't allow to customise name though, but that was never part of the issue you opened so I guess we can leave that out for now 👍

[col]

Yeah that looks good. Think about making name an option as well. Will update shortly. Want me to update the example usage in the readme to include the pool_size option?

…

On Fri, 23 Nov 2018 at 10:03 pm, Mattia ***@***.***> wrote: ***@***.**** requested changes on this pull request. Good test coverage, however I believe the implementation can be improved and simplified a bit. Please see my comment below and let me know your thoughts. ------------------------------ In lib/faraday/adapter/net_http_persistent.rb <#834 (comment)>: > @@ -8,7 +8,9 @@ class NetHttpPersistent < NetHttp def net_http_connection(env) @cached_connection ||= if Net::HTTP::Persistent.instance_method(:initialize).parameters.first == [:key, :name] - Net::HTTP::Persistent.new(name: 'Faraday') + options = {} + options[:pool_size] = @connection_options[:pool_size] if @connection_options.key?(:pool_size) + Net::HTTP::Persistent.new(name: 'Faraday', **options) The above can be rewritten as: options = {name: ***@***.***_options.slice(:name, :pool_size)Net::HTTP::Persistent.new(options) This way we: - Simplify the filtering using slice - Allow to override the connection name proxy would also be a valid key, but I'd not slice it in as Faraday manages the proxy separately. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#834 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAN0_usHxek-SHxfi3F9YL5Ny4G3ft1tks5uyAA_gaJpZM4Ywrl0> .

[iMacTia]

Why not, sounds like a great idea 😃