[Tarantool-patches] [PATCH v2 0/2] console: support of backslash

Alexander Turenko alexander.turenko at tarantool.org
Wed Jul 8 21:42:35 MSK 2020


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


More information about the Tarantool-patches mailing list