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

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

[Kirill Shcherbatov]

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
+--
+-- This makes possible do box.sql.execute without calling box.cfg
+-- manually. The load_cfg call would overwrite following table and
+-- metatable.
+--
+box.sql = {}
+setmetatable(box.sql, {
+    __index = function(table, index)
+        load_cfg()
+        return box.sql[index]
+    end,
+})
+
 -- gh-810:
 -- hack luajit default cpath
 -- commented out because we fixed luajit to build properly, see
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
+-- sql.cfg() call
+--
+box.cfg()
+---
+...
+assert(next(box.sql) ~= box.NULL)
+---
+- true
+...
+assert(getmetatable(box.sql) == box.NULL)
+---
+- true
+...
 test_run:cmd("clear filter")
 ---
 - true
diff --git a/test/box/cfg.test.lua b/test/box/cfg.test.lua
index 2d819c9..b1f4dc9 100644
--- a/test/box/cfg.test.lua
+++ b/test/box/cfg.test.lua
@@ -91,4 +91,14 @@ box.cfg{net_msg_max = 2}
 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
+-- sql.cfg() call
+--
+box.cfg()
+assert(next(box.sql) ~= box.NULL)
+assert(getmetatable(box.sql) == box.NULL)
+
 test_run:cmd("clear filter")
-- 
2.7.4

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

[Vladislav Shpilevoy]

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.

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

[Kirill Shcherbatov]

> 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")

[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")
> 

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

[n.pettik]

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

I am OK as well.

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

[Kirill Yukhin]

Hello,
On 18 мая 14:39, n.pettik wrote:
> 
> > Now the patch LGTM. Nikita, review the patch please.
> 
> I am OK as well.
I've committed this patch to 2.0 branch.

Kirill, could you pls use prefixed branch names as stated in SOP?

gh-3266-no-box-cfg-in-sql -> ks/gh-3266-no-box-cfg-in-sql?

--
Regards, Kirill Yukhin