Add a callback module for injecting config by other means #104

srenatus · 2017-02-23T09:37:14Z

...defaulting to what it was doing before: using envy:get/4 to fetch
it from the app environment.

seth · 2017-02-23T17:35:56Z

src/sqerl_client.erl

@@ -241,19 +212,29 @@ drivermod() ->
            log_and_error({invalid_application_config, sqerl, db_driver_mod, Error})
    end.

+%% @doc Returns the config, based on configured config_cb MFA. Defaults to
+%% using squerl_config_env:config/0, which will use the application environment.


Of course! 😄 Thanks

seth · 2017-02-23T17:40:49Z

src/sqerl_client.erl

-              {idle_check, IdleCheck},
-              {prepared_statements, Statements},
-              {column_transforms, envy:get(sqerl, column_transforms, list)}],
+init(CallbackMod, Config) ->


Reading the PR I got confused because the title of the change is "callback mod" but this is the driver module, not the config one. I realize not related to this change, but wonder if a variable name change to DriverMod would be a readability improvement.

seth · 2017-02-23T17:42:19Z

src/sqerl_client.erl

-              {prepared_statements, Statements},
-              {column_transforms, envy:get(sqerl, column_transforms, list)}],
+init(CallbackMod, Config) ->
+    IdleCheck = proplists:get_value(idle_check, Config),


Do we handle undefined for IdleCheck? If not, consider verifying or crashing? Perhaps lists:keyfind?

I've gone with a default, similar to what the record define has.

seth · 2017-02-23T17:46:07Z

src/sqerl_client.erl

+    {M, F, A} = case envy:get(sqerl, config_cb, undefined, any) of
+        undefined ->
+            {sqerl_config_env, config, []};
+        {Mod, Fun, Args} = MFA when is_atom(Mod) andalso


I think this is probably ok since only devs are going to use the feature, but otherwise a friendlier error message when wrong type is provided would make sense. Agree?

...defaulting to what it was doing before: using envy:get/4 to fetch it from the app environment. Signed-off-by: Stephan Renatus <srenatus@chef.io>

This follows the intent of the record default value. Signed-off-by: Stephan Renatus <srenatus@chef.io>

srenatus · 2017-02-23T19:21:08Z

@seth thanks for the feedback -- I've pushed another commit.

Your comments got me thinking, given this behaviour of envy:get/4,

$ rebar3 shell
===> Verifying dependencies...
===> Compiling sqerl
Erlang/OTP 18 [erts-7.3] [source] [64-bit] [smp:8:8] [async-threads:0] [hipe] [kernel-poll:false]

Eshell V7.3  (abort with ^G)
1> application:set_env(sqerl, db_driver_mod, {foo, bar}).
ok
2> envy:get(sqerl, db_driver_mod, undefined, atom).

=ERROR REPORT==== 23-Feb-2017::20:16:33 ===
Bad typecheck for config item for 'sqerl' 'db_driver_mod' (atom({foo,bar}) -> false)
** exception error: config_bad_type
     in function  envy:get_validate/4 (/Users/stephan/Src/chef/sqerl/_build/default/lib/envy/src/envy.erl, line 105)
3>

would this Error case

    case envy:get(sqerl, db_driver_mod, undefined, atom) of
        undefined ->
            %% ...
        DriverMod when is_atom(DriverMod) ->
            %% ...
        Error ->
            log_and_error({invalid_application_config, sqerl, db_driver_mod, Error})
    end.

(and that one, too) ever happen? It seems like it would need a try/catch... 🤔 (But I might as well have overlooked something at the moment...)

Update

Yes! Too tired! Of course it can happen in the new code, application:set_env(sqerl, config_cb, {"foo", "bar", 0}) Please disregard ^

I think the new commit still makes sense, though. I'd suppose the try/catch is too defensive, since if that validation fails, we'll get a good-enough feedback.

srenatus force-pushed the ssd-sr/callback-mod-for-config branch 3 times, most recently from 31e18cc to 7d7ce4f Compare February 23, 2017 12:58

seth reviewed Feb 23, 2017

View reviewed changes

srenatus added 2 commits February 23, 2017 20:13

Add a callback module for injecting config by other means

19f84e1

...defaulting to what it was doing before: using envy:get/4 to fetch it from the app environment. Signed-off-by: Stephan Renatus <srenatus@chef.io>

Add default timeout to IdleCheck setup, improve error

220bb1c

This follows the intent of the record default value. Signed-off-by: Stephan Renatus <srenatus@chef.io>

srenatus force-pushed the ssd-sr/callback-mod-for-config branch from 7d7ce4f to 220bb1c Compare February 23, 2017 19:14

seth approved these changes Feb 23, 2017

View reviewed changes

stevendanna approved these changes Feb 24, 2017

View reviewed changes

stevendanna merged commit 2d9792c into master Feb 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a callback module for injecting config by other means #104

Add a callback module for injecting config by other means #104

srenatus commented Feb 23, 2017

seth Feb 23, 2017

srenatus Feb 23, 2017

seth Feb 23, 2017

seth Feb 23, 2017

srenatus Feb 23, 2017

seth Feb 23, 2017

srenatus commented Feb 23, 2017 •

edited

Loading

Add a callback module for injecting config by other means #104

Add a callback module for injecting config by other means #104

Conversation

srenatus commented Feb 23, 2017

seth Feb 23, 2017

Choose a reason for hiding this comment

srenatus Feb 23, 2017

Choose a reason for hiding this comment

seth Feb 23, 2017

Choose a reason for hiding this comment

seth Feb 23, 2017

Choose a reason for hiding this comment

srenatus Feb 23, 2017

Choose a reason for hiding this comment

seth Feb 23, 2017

Choose a reason for hiding this comment

srenatus commented Feb 23, 2017 • edited Loading

srenatus commented Feb 23, 2017 •

edited

Loading