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) >
next prev parent 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