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

[Vladislav Shpilevoy]

Now the patch LGTM. Nikita, review the patch please.

On 16/05/2018 16:22, Kirill Shcherbatov wrote:
>> 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).
> 
> -- a/src/box/lua/load_cfg.lua
> +++ b/src/box/lua/load_cfg.lua
> @@ -419,10 +419,8 @@ end
> box.cfg = load_cfg
> 
> --
> --- gh-3266: box.cfg{} still not optional on 2.0 brach
> ---
> -- This makes possible do box.sql.execute without calling box.cfg
> --- manually. The load_cfg call would overwrite following table and
> +-- manually. The load_cfg call overwrites following table and
> -- metatable.
> --
> box.sql = {}
> diff --git a/test/box/cfg.result b/test/box/cfg.result
> index 42a0a89..b718056 100644>
> 
>> 3. overridden.
>> 4. sql.cfg -> box.cfg.
>> 5. Why box.NULL? NULL is cdata, and used mostly to insert NULL into
>> 6. Same.
> 
> --- a/test/box/cfg.result
> +++ b/test/box/cfg.result
> @@ -437,17 +437,17 @@ 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
> --- sql.cfg() call
> +-- box.sql defined with __index function in metatable overridden
> +-- with first box.cfg() call
> --
> box.cfg()
> ---
> ...
> -assert(next(box.sql) ~= box.NULL)
> +assert(next(box.sql) ~= nil)
> ---
> - true
> ...
> -assert(getmetatable(box.sql) == box.NULL)
> +assert(getmetatable(box.sql) == nil)
> ---
> - true
> ...
> diff --git a/test/box/cfg.test.lua b/test/box/cfg.test.lua
> index b1f4dc9..01a6ccd 100644
> --- a/test/box/cfg.test.lua
> +++ b/test/box/cfg.test.lua
> @@ -94,11 +94,11 @@ 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
> --- sql.cfg() call
> +-- box.sql defined with __index function in metatable overridden
> +-- with first box.cfg() call
> --
> box.cfg()
> -assert(next(box.sql) ~= box.NULL)
> -assert(getmetatable(box.sql) == box.NULL)
> +assert(next(box.sql) ~= nil)
> +assert(getmetatable(box.sql) == nil)
> 
> test_run:cmd("clear filter")
>