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 84A3D2B809 for ; Thu, 11 Apr 2019 10:57:35 -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 XOeO7YU44R4n for ; Thu, 11 Apr 2019 10:57:35 -0400 (EDT) Received: from smtp16.mail.ru (smtp16.mail.ru [94.100.176.153]) (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 B66482B796 for ; Thu, 11 Apr 2019 10:57:34 -0400 (EDT) Date: Thu, 11 Apr 2019 17:57:26 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: [PATCH] tarantoolctl: remove metatable assumptions in start() Message-ID: <20190411145725.il4veadyw4lcktj2@tkn_work_nb> References: <20190327134014.56676-1-roman.habibov@tarantool.org> <20190401054304.kofvdg3uz6fzwl5d@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: 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: Roman Khabibov Cc: tarantool-patches@freelists.org The behaviour is almost okay, but you need to work on wording of the commit message to make it useful for a reader. Feel free to ask more if you have questions. See comments below. WBR, Alexander Turenko. > commit 4cdf8e79bb530288098fb30e5014498fc0ad4262 > Author: Roman Khabibov > Date: Mon Mar 25 20:52:05 2019 +0300 > > tarantoolctl: error when box.cfg didn't called Cite from our developer guidelines [1]: > 6. Use the imperative mood in the subject line. A properly formed Git > commit subject line should always be able to complete the following > sentence: “If applied, this commit will /your subject line here/”. [1]: https://www.tarantool.io/en/doc/1.10/dev_guide/developer_guidelines/#how-to-write-a-commit-message Also you construct passive voice wrongly, it should be 'to be + [not] + Ved'. > > There was assumpted uninitialized metamethods of box.cfg{} if user > did't call it in his instance. This sentence raises more questions then give answers. What is 'uninitialized metamethods'? Whether 'of box.cfg{}' means result of the call or a new value of the 'cfg' field of the 'box' table? Also I doubt 'There was assumpted' is valid phrase. > > Closes #3953 > > diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in > index 47fcf895f..396fa7957 100755 > --- a/extra/dist/tarantoolctl.in > +++ b/extra/dist/tarantoolctl.in > @@ -547,7 +547,11 @@ local function start() > end > os.exit(1) > end > - local old_call = getmetatable(box.cfg).__call > + local m_table = getmetatable(box.cfg) > + if m_table == nil then Cite from our Lua style guide [2]: > `*_mt` and `*_methods` defines metatable and methods table Suggested to name it box_cfg_mt. [2]: https://www.tarantool.io/en/doc/1.10/dev_guide/lua_style_guide/ > + error("Plaese, call box.cfg{} within your instance") Typo: 'Plaese'. I would change wording to just state what is wrong, e.g.: 'box.cfg() is not called in an instance file'. See also how errors are reported in other places: log.error() and os.exit(1). I guess it does not matter much from a behaviour point of view, but it would be good to report errors in one way with surrounding code. (BTW, found one behaviour difference: error() reports a line number when called with one argument or non-zero second argument.) Please, install tarantool and tarantoolctl and check the behaviour: messages and exit codes. Currently it looks so: # tarantoolctl start foo Starting instance foo... Hello, World! /usr/bin/tarantoolctl:552: Plaese, call box.cfg{} within your instance # echo $? 1 It is okay except file:line part, but anyway I suggest you to use log.error() and os.exit(1). > + end > + local old_call = m_table.__call > getmetatable(box.cfg).__call = function(old_cfg, cfg) There is no need to call getmetatable() again, we can use box_cfg_mt. > if old_cfg.pid_file ~= nil and cfg ~= nil and cfg.pid_file ~= nil then > cfg.pid_file = old_cfg.pid_file > diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua > index db046e03f..428b074e2 100755 > --- a/test/app-tap/tarantoolctl.test.lua > +++ b/test/app-tap/tarantoolctl.test.lua > @@ -165,10 +165,13 @@ do > local dir = fio.tempdir() > local code = [[ box.cfg{memtx_memory = 104857600} ]] > create_script(dir, 'script.lua', code) > + create_script(dir, 'init.lua', [[ print('Hi!') ]]) > > local status, err = pcall(function() > test:test("basic test", function(test_i) > - test_i:plan(16) > + test_i:plan(18) > + check_ok(test_i, dir, 'start', 'init.lua', 1, nil, "Starting instance init...", > + "Plaese, call box.cfg{} within your instance") > check_ok(test_i, dir, 'start', 'script', 0, nil, "Starting instance") > tctl_wait_start(dir, 'script') > check_ok(test_i, dir, 'status', 'script', 0, nil, "is running") > > I propose to make the code of the test a bit more uniform and use a meaningful instance file name: diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua index 428b074e2..7d7c63371 100755 --- a/test/app-tap/tarantoolctl.test.lua +++ b/test/app-tap/tarantoolctl.test.lua @@ -163,15 +163,14 @@ test:plan(7) -- must be stopped afterwards do local dir = fio.tempdir() - local code = [[ box.cfg{memtx_memory = 104857600} ]] - create_script(dir, 'script.lua', code) - create_script(dir, 'init.lua', [[ print('Hi!') ]]) + create_script(dir, 'script.lua', [[ box.cfg{memtx_memory = 104857600} ]]) + create_script(dir, 'no_box_cfg.lua', [[ print('Hi!') ]]) local status, err = pcall(function() test:test("basic test", function(test_i) test_i:plan(18) - check_ok(test_i, dir, 'start', 'init.lua', 1, nil, "Starting instance init...", - "Plaese, call box.cfg{} within your instance") + check_ok(test_i, dir, 'start', 'no_box_cfg', 1, nil, "Starting instance", + "box.cfg() is not called in an instance file") check_ok(test_i, dir, 'start', 'script', 0, nil, "Starting instance") tctl_wait_start(dir, 'script') check_ok(test_i, dir, 'status', 'script', 0, nil, "is running")