Warning when specifiying custom middleware list

[marcotc]

While writing test for an excon middleware, I came across this warning:

[excon][WARNING] Invalid Excon connection keys: :idempotent, :instrumentor_name, :mock, :retry_errors, :retry_limit
/Users/marco.costa/.rbenv/versions/2.5.1/lib/ruby/gems/2.5.0/gems/excon-0.66.0/lib/excon/connection.rb:425:in `validate_params'
/Users/marco.costa/.rbenv/versions/2.5.1/lib/ruby/gems/2.5.0/gems/excon-0.66.0/lib/excon/connection.rb:71:in `initialize'
/Users/marco.costa/.rbenv/versions/2.5.1/lib/ruby/gems/2.5.0/gems/excon-0.66.0/lib/excon.rb:142:in `new'
/Users/marco.costa/.rbenv/versions/2.5.1/lib/ruby/gems/2.5.0/gems/excon-0.66.0/lib/excon.rb:142:in `new'
(pry):3:in `__pry__'

This warning can be reproduced with the following snippet:

$VERBOSE = true
require 'excon'
Excon.new('http://httpstat.us/200', middlewares: [Excon::Middleware::ResponseParser]).get;

This warning is issued because the list of middlewares I provided to excon is a subset of Excon.defaults[:middlewares], and Excon.defaults includes :idempotent, :instrumentor_name, :mock, :retry_errors, and :retry_limit as default parameters. Because their respective middlewares are not registered, excon thinks these are extraneous parameters.

Here's the line responsible for this validation.

[marcotc]

One suggestion I have is to add default middleware parameters to the default validation list.

valid_middleware_keys(middlewares) + Excon::VALID_CONNECTION_KEYS + Excon::DEFAULT_MIDDLEWARE_PARAMETER_KEYS

[geemus]

Thanks for the heads up. I suspect a better fix might be that just as middlewares inject valid keys when they are included, that they would also inject defaults for themself? This seems a bit more complicated, but also more future proof/extensible. What do you think?

[marcotc]

@geemus default values for middleware keys seem like a great approach!

Given that Excon::Middleware::Base.valid_parameter_keys is already in the public, I'm guessing adding another method to Excon::Middleware::Base that returns a hash with keys and default values would be less impactful than breaking the existing middleware contract.

[stale]

This issue has been automatically marked stale due to inactivity. It will be closed if no further activity occurs. Thank you for your contributions.

[geemus]

Pinned, I do intend to circle back to this at some point, just haven't been able to make it a priority.