From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp21.mail.ru (smtp21.mail.ru [94.100.179.250]) (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 D7431445320 for ; Tue, 28 Jul 2020 19:42:23 +0300 (MSK) Date: Tue, 28 Jul 2020 19:41:28 +0300 From: Alexander Turenko Message-ID: <20200728164128.7sbgbjw3qiayuouc@tkn_work_nb> References: <1f495519687c8e037c638ccabab28e23882a41df.1595943364.git.sergeyb@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1f495519687c8e037c638ccabab28e23882a41df.1595943364.git.sergeyb@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH 1/2] src: return back import of table.clear() method List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: sergeyb@tarantool.org Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org > src: return back import of table.clear() method We usually mark a subsystem using a prefix: 'app' or 'lua' would fit good here. BTW, why you splitted the code and test changes? I know, there are cons and pros, but we usually include tests into the same commit as a source change. Upside: It works as the documentation of changes; a formal description of the change in addition to informal one within a commit message. Downside: A bit more work is required to verify the test on different tarantool version. > Import of 'table.clear' module has been removed > to fix luacheck warning about unused variable in > commit 3af79e70b5e1e9b1d69b97f3031a299132a02d2f Nit: We usually use form git_commit_hash ('header of the commit') when mention a commit. > and method table.clear() became unavailable in Tarantool. I would clarify that `table.clear` is not available until an explicit `require('table.clear')` call. > --- a/src/lua/trigger.lua > +++ b/src/lua/trigger.lua > @@ -1,4 +1,5 @@ > local fun = require('fun') > +local _ = require('table.clear') I would add a comment that would explain why it is necessary. Just to don't confuse a reader of the code. It seems it was added non-intentionally in 8a3dae66f6f2c8dffac39bbda1b38d1e17dfcc63 ('On schema reload callback for net.box module') (not used anywhere in sources). However now we see that the membership module leans on the fact that it is available without explicit 'require'. I would look for more appropriate place for it: there is nothing special in trigger.lua. Maybe src/lua/table.lua or src/lua/init.lua. The former looks better for me. WBR, Alexander Turenko.