Tarantool development patches archive
 help / color / mirror / Atom feed
From: Alexander Turenko <alexander.turenko@tarantool.org>
To: Roman Khabibov <roman.habibov@tarantool.org>
Cc: tarantool-patches@freelists.org
Subject: [tarantool-patches] Re: [PATCH] tarantoolctl: remove metatable assumptions in start()
Date: Thu, 11 Apr 2019 17:57:26 +0300	[thread overview]
Message-ID: <20190411145725.il4veadyw4lcktj2@tkn_work_nb> (raw)
In-Reply-To: <AE7E3E15-7F04-4494-9384-717ABC637273@tarantool.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 <roman.habibov@tarantool.org>
> 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")

  reply	other threads:[~2019-04-11 14:57 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-27 13:40 [tarantool-patches] " Roman Khabibov
2019-03-28  8:59 ` [tarantool-patches] " Konstantin Osipov
2019-04-01  5:43 ` Alexander Turenko
2019-04-05 23:26   ` Roman Khabibov
2019-04-11 14:57     ` Alexander Turenko [this message]
2019-04-12  9:29       ` Roman Khabibov
2019-04-14 22:41         ` Alexander Turenko
2019-04-15 10:59           ` Roman Khabibov
2019-04-15 12:55             ` Alexander Turenko
2019-04-16 12:01 ` Kirill Yukhin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190411145725.il4veadyw4lcktj2@tkn_work_nb \
    --to=alexander.turenko@tarantool.org \
    --cc=roman.habibov@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH] tarantoolctl: remove metatable assumptions in start()' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox