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