[tarantool-patches] Re: [PATCH v1 1/1] box: ability to omit box.cfg() call in sql

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hello. Thanks for the short and simple patch! See my 6 minor comments below.

On 16/05/2018 15:51, Kirill Shcherbatov wrote:
> This makes possible to call box.sql.execute without pre-calling box.cfg.
> Introduced metatable __index function for box.sql that  called when no
> appropriate box.sql methods (execute, debug) is defined. It calls
> box.cfg() itself that redefines box.sql table.
> 
> Closes #3266
> ---
>   src/box/lua/load_cfg.lua | 15 +++++++++++++++
>   test/box/cfg.result      | 17 +++++++++++++++++
>   test/box/cfg.test.lua    | 10 ++++++++++
>   3 files changed, 42 insertions(+)
> 
> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
> index 4ace1e1..781de3e 100644
> --- a/src/box/lua/load_cfg.lua
> +++ b/src/box/lua/load_cfg.lua
> @@ -418,6 +418,21 @@ local function load_cfg(cfg)
>   end
>   box.cfg = load_cfg
>   
> +--
> +-- gh-3266: box.cfg{} still not optional on 2.0 brach

1. Please, do not reference github issues in the code. Only in tests.
I see, that it is ignored few lines above, but it was wrong too. Unfortunately,
the author of this '   -- gh-810:' line sometimes ignores its own rules.

> +--
> +-- This makes possible do box.sql.execute without calling box.cfg
> +-- manually. The load_cfg call would overwrite following table and
> +-- metatable.

2. Why 'would'? It actually overrides (it is ok, only the comment is strange).

> diff --git a/test/box/cfg.result b/test/box/cfg.result
> index 14dceac..42a0a89 100644
> --- a/test/box/cfg.result
> +++ b/test/box/cfg.result
> @@ -434,6 +434,23 @@ box.cfg{net_msg_max = old + 1000}
>   box.cfg{net_msg_max = old}
>   ---
>   ...
> +--
> +-- gh-3266: box.cfg{} still not optional on 2.0 brach
> +--
> +-- box.sql defined with __index function in metatable overriten with first

3. overridden.

> +-- sql.cfg() call

4. sql.cfg -> box.cfg.

> +--
> +box.cfg()
> +---
> +...
> +assert(next(box.sql) ~= box.NULL)

5. Why box.NULL? NULL is cdata, and used mostly to insert NULL into
spaces. Lets use nil.

> +---
> +- true
> +...
> +assert(getmetatable(box.sql) == box.NULL)

6. Same.