-
Notifications
You must be signed in to change notification settings - Fork 18
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
allow master defaults in connect #19
Conversation
@nalundgaard take a look. purposed on this is straight forward. motivation simplify life for erlcloud users like us |
src/lhttpc_client.erl
Outdated
infinity), | ||
connect_options = proplists:get_value(connect_options, Options, []), | ||
connect_timeout = proplists:get_value(connect_timeout, Options, DefConTimeout), | ||
connect_options = proplists:get_value(connect_options, Options, DefConOptions), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
connect_options
is a list. so for me it looks natural to merge DefConOptions
and connect_options
from Options
, but not drop all DefConOptions
in case any option is passed to execute()
call.
current version will increase entropy of the universe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair enough. agree. will make it soon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 here.
src/lhttpc_client.erl
Outdated
@@ -150,6 +150,9 @@ execute(From, Host, Port, Ssl, Path, Method, Hdrs0, Body, Options) -> | |||
%% Get a socket for the pool or exit | |||
%Socket = lhttpc_manager:ensure_call(Pool, SocketRequest, Options), | |||
Socket = lhttpc_manager:ensure_call(Pool, self(), Host, Port, Ssl, Options), | |||
% get the socket connect settings from client or use defaults | |||
DefConOptions = application:get_env(lhttpc, connect_options, []), | |||
DefConTimeout = proplists:get_value(DefConOptions, connect_timeout, infinity), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks backwards. Please change to proplists:get_value(connect_timeout, DefConOptions, infinity)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(doh)
src/lhttpc_client.erl
Outdated
infinity), | ||
connect_options = proplists:get_value(connect_options, Options, []), | ||
connect_timeout = proplists:get_value(connect_timeout, Options, DefConTimeout), | ||
connect_options = proplists:get_value(connect_options, Options, DefConOptions), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 here.
tested as
|
* Adds support for defaulting lhttpc connect_options via the OTP app configuration; see erlcloud/lhttpc#19
allow setting defaults for connect options once in config and note upon every request.