From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp5.mail.ru (smtp5.mail.ru [94.100.179.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 83686445320 for ; Wed, 8 Jul 2020 21:43:22 +0300 (MSK) Date: Wed, 8 Jul 2020 21:42:35 +0300 From: Alexander Turenko Message-ID: <20200708184235.pdw3qdlu5twdbiv4@tkn_work_nb> References: <20200610094354.31831-1-arkholga@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200610094354.31831-1-arkholga@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2 0/2] console: support of backslash List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Olga Arkhangelskaia Cc: tarantool-patches@dev.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