From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id C4F9E24FA3 for ; Thu, 17 May 2018 05:29:49 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id WTPaR0OMHLyR for ; Thu, 17 May 2018 05:29:49 -0400 (EDT) Received: from smtp18.mail.ru (smtp18.mail.ru [94.100.176.155]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 7ED0324F9B for ; Thu, 17 May 2018 05:29:49 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v1 1/1] box: ability to omit box.cfg() call in sql References: <6056f62646b1aee1ce8c99b7d3fe5cc3d78a993d.1526475084.git.kshcherbatov@tarantool.org> <52ef9ff2-8b64-d68d-23d4-757303680135@tarantool.org> <1f99c046-1422-a18a-eb65-ff6e1cf6adba@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Thu, 17 May 2018 12:29:45 +0300 MIME-Version: 1.0 In-Reply-To: <1f99c046-1422-a18a-eb65-ff6e1cf6adba@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Kirill Shcherbatov , tarantool-patches@freelists.org, Nikita Pettik 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") >