Tarantool development patches archive
 help / color / mirror / Atom feed
From: Alexander Turenko <alexander.turenko@tarantool.org>
To: Olga Arkhangelskaia <arkholga@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH] test_run: add path option to grep_log
Date: Wed, 4 Mar 2020 16:51:33 +0300	[thread overview]
Message-ID: <20200304135133.hnj3wdsnizsbo76l@tkn_work_nb> (raw)
In-Reply-To: <20200218180101.37385-1-arkholga@tarantool.org>

The patch looks okay, but I would ask you to do few small modifications
(see below).

WBR, Alexander Turenko.

On Tue, Feb 18, 2020 at 09:01:01PM +0300, Olga Arkhangelskaia wrote:
> While using grep_log it is useful to have a possibility to go through crashed

Please, keep a commit message body within 72 symbols.

See https://www.tarantool.io/en/doc/1.10/dev_guide/developer_guidelines/#how-to-write-a-commit-message

> instance log.
> Usage in case of crashed instance:
> test_run:grep_log(nil, str-to-find, nil, {path=path_to_log})
> 
> Closes #196
> ---
> Issue:https://github.com/tarantool/test-run/issues/196                          
> Branch:https://github.com/tarantool/test-run/pull/new/OKriw/gh-196-Extend-grep_log-for-crashed-instances
> 
>  test_run.lua | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/test_run.lua b/test_run.lua
> index 4d8132d..68dba53 100644
> --- a/test_run.lua
> +++ b/test_run.lua
> @@ -326,7 +326,9 @@ end
>  local function grep_log(self, node, what, bytes, opts)
>      local opts = opts or {}
>      local noreset = opts.noreset or false
> -    local filename = self:eval(node, "box.cfg.log")[1]
> +    -- node is used in case of alive instance, otherwise path to log can be
> +    -- used for path
> +    local filename = opts.path or self:eval(node, "box.cfg.log")[1]

Nit: It is rule of thumb to fit comments within 66 symbols.

We already use 'filename' name for this, I would use it for the option
too.

I suggest the following pattern for options:

 | local function f(<...>, opts)
 |     local opts = opts or {}
 |     local foo = opts.foo (or <default_value> if it is not nil)
 |     local bar = opts.bar
 |     <...the rest code...>
 | end

This way it is easy to find them all in code at one glance.

>      local file = fio.open(filename, {'O_RDONLY', 'O_NONBLOCK'})
>  
>      local function fail(msg)
> -- 
> 2.20.1 (Apple Git-117)
> 

  reply	other threads:[~2020-03-04 13:51 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-18 18:01 Olga Arkhangelskaia
2020-03-04 13:51 ` Alexander Turenko [this message]
2020-03-10 11:29   ` Olga Arkhangelskaia
2020-03-13  7:36     ` Alexander Turenko
     [not found]     ` <20200316091553.4hbyadvcubrpdzk4@tkn_work_nb>
2020-03-16  9:18       ` Alexander Turenko

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=20200304135133.hnj3wdsnizsbo76l@tkn_work_nb \
    --to=alexander.turenko@tarantool.org \
    --cc=arkholga@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH] test_run: add path option to grep_log' \
    /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