Re: [Tarantool-patches] [PATCH 1/2] src: return back import of table.clear() method

[Sergey Bronnikov <sergeyb@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@