From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp61.i.mail.ru (smtp61.i.mail.ru [217.69.128.41]) (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 5E7A543040D for ; Mon, 31 Aug 2020 14:14:25 +0300 (MSK) Date: Mon, 31 Aug 2020 14:14:24 +0300 From: Sergey Bronnikov Message-ID: <20200831111424.GC94343@pony.bronevichok.ru> References: <1f495519687c8e037c638ccabab28e23882a41df.1595943364.git.sergeyb@tarantool.org> <20200728164128.7sbgbjw3qiayuouc@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200728164128.7sbgbjw3qiayuouc@tkn_work_nb> 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: Alexander Turenko Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org Alexander, thanks for review! See my comments inline. Patch has been updated in a branch and squashed to a single commit. On 19:41 Tue 28 Jul , Alexander Turenko wrote: > > src: return back import of table.clear() method > > We usually mark a subsystem using a prefix: 'app' or 'lua' would fit > good here. set prefix 'lua' in updated commit message > 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. >From my point of view it is better to split functional changes and tests. Agree with you that there are different pros et contras regarding it and I don't want to argue about it. I have squashed changes to a single commit. > 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. Done. > > and method table.clear() became unavailable in Tarantool. > > I would clarify that `table.clear` is not available until an explicit > `require('table.clear')` call. Done in commit message. > > --- 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. Done: local table = require('table') +-- require modifies global "table" module and adds "clear" function to it. +-- Lua applications like Cartridge relies on it. +local _ = require('table.clear') + table.copy = table_shallowcopy > 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. I chose a file src/lua/table.lua and moved module import to it. > WBR, Alexander Turenko. -- sergeyb@