[tarantool-patches] Re: [PATCH v1 1/1] box: ability to omit box.cfg() call in sql
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Wed May 16 16:10:35 MSK 2018
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.
More information about the Tarantool-patches
mailing list