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 v2 0/2] console: support of backslash
Date: Wed, 8 Jul 2020 21:42:35 +0300	[thread overview]
Message-ID: <20200708184235.pdw3qdlu5twdbiv4@tkn_work_nb> (raw)
In-Reply-To: <20200610094354.31831-1-arkholga@tarantool.org>

First, I don't see any updates in the mailing list since v2, where Igor
and me responds. However the branch is updated and I'll left a couple of
comment regarding the patch on the branch.

Typo: tatantool.

Please, use spell checker. I don't remember a patch without typos from
you.

Wrong indent in console.lua.

No empty line between client_read() and client_print().

gh-4317.test.lua -- please, follow our naming guidelines.

CI fails on Mac OS X (see [1]):

 | [017] app-tap/gh-4317.test.lua
 | [017] TAP13 parse failed (Missing plan in the TAP source).

Move all requires to the top of the lua file (in test).

Run luacheck on your test file. Several warnings are relevant to our
guidelines: say, about using local (see [2]) or usnused variables.
However we agreed that some warnings are should be considered
false-positive for us: say, variables hiding and redefinitions.

Remove print().

Remove a space before a colon.

----

I don't ever started to look into the logic of changes and already has a
half screen of comments. It is not acceptable and just wastes my time.

If you'll not do thorough self-review (at least points mentioned in to
[3]), I'll not do review for you. Don't take it personal, it is how we
work in the team.

I can do review for the test, but console.lua changes are partially
written by me, so I hope Igor will do review for this part (when you'll
send the next version).

So, the plan is the following:

- Rebase to master, fix test of Mac OS, **do self-review carefully**.
  Ask questions what is considered good style and bad if it is unclear
  (ask in the team chat or from me directly).
- Send v3 to Igor and me.

The ball is in your court.

WBR, Alexander Turenko.

[1]: https://gitlab.com/tarantool/tarantool/-/jobs/629030801
[2]: https://github.com/tarantool/doc/issues/1004
[3]: https://github.com/tarantool/tarantool/wiki/Code-review-procedure

  parent reply	other threads:[~2020-07-08 18:43 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-10  9:43 Olga Arkhangelskaia
2020-06-10  9:43 ` [Tarantool-patches] [PATCH 1/2] test: add libisatty to test local console Olga Arkhangelskaia
2020-06-11 16:47   ` Igor Munkin
2020-06-10  9:43 ` [Tarantool-patches] [PATCH 2/2] console: add line carrying backslash Olga Arkhangelskaia
2020-06-11 16:48   ` Igor Munkin
2020-06-16 14:39   ` Alexander Turenko
2020-06-11 16:49 ` [Tarantool-patches] [PATCH v2 0/2] console: support of backslash Igor Munkin
2020-07-08 18:42 ` Alexander Turenko [this message]
2020-07-09  6:01   ` Olga Arkhangelskaia

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=20200708184235.pdw3qdlu5twdbiv4@tkn_work_nb \
    --to=alexander.turenko@tarantool.org \
    --cc=arkholga@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v2 0/2] console: support of backslash' \
    /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