[Tarantool-patches] [PATCH 0/2] Recover of table.clear() method

[Tarantool-patches] [PATCH 0/2] Recover of table.clear() method

[sergeyb]

From: Sergey Bronnikov <sergeyb@tarantool.org>

Patch [1] introduced regression -
import of 'table.clear' module has been removed. This patch series
fix a bug and adds a test to clear a table.

1. https://github.com/tarantool/tarantool/commit/3af79e70b5e1e9b1d69b97f3031a299132a02d2f

GitHub issue: https://github.com/tarantool/tarantool/issues/5210
GitLab pipeline: https://gitlab.com/tarantool/tarantool/-/pipelines/171616899

Sergey Bronnikov (2):
  src: return back import of table.clear() method
  test: add regression test for table.clear()

 src/lua/trigger.lua                       |  1 +
 test/box-tap/gh-5210-table-clear.test.lua | 10 ++++++++++
 2 files changed, 11 insertions(+)
 create mode 100755 test/box-tap/gh-5210-table-clear.test.lua

-- 
2.26.2

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

[sergeyb]

From: Sergey Bronnikov <sergeyb@tarantool.org>

Import of 'table.clear' module has been removed
to fix luacheck warning about unused variable in
commit 3af79e70b5e1e9b1d69b97f3031a299132a02d2f
and method table.clear() became unavailable in Tarantool.

Part of #5210
---
 src/lua/trigger.lua | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/lua/trigger.lua b/src/lua/trigger.lua
index 1330ecdd4..066329ea6 100644
--- a/src/lua/trigger.lua
+++ b/src/lua/trigger.lua
@@ -1,4 +1,5 @@
 local fun = require('fun')
+local _ = require('table.clear')
 
 --
 -- Checks that argument is a callable, i.e. a function or a table
-- 
2.26.2

[Tarantool-patches] [PATCH 2/2] test: add regression test for table.clear()

[sergeyb]

From: Sergey Bronnikov <sergeyb@tarantool.org>

Closes #5210
---
 test/box-tap/gh-5210-table-clear.test.lua | 10 ++++++++++
 1 file changed, 10 insertions(+)
 create mode 100755 test/box-tap/gh-5210-table-clear.test.lua

diff --git a/test/box-tap/gh-5210-table-clear.test.lua b/test/box-tap/gh-5210-table-clear.test.lua
new file mode 100755
index 000000000..88f773b22
--- /dev/null
+++ b/test/box-tap/gh-5210-table-clear.test.lua
@@ -0,0 +1,10 @@
+#!/usr/bin/env tarantool
+
+local tap = require('tap')
+local test = tap.test('gh-5210-table-clear')
+
+test:plan(1)
+t = {a = 1, b = 2}
+test:is(table.clear(t), nil, 'table clear')
+
+os.exit(test:check() and 0 or 1)
-- 
2.26.2

Re: [Tarantool-patches] [PATCH 0/2] Recover of table.clear() method

[Alexander Turenko]

> GitHub issue: https://github.com/tarantool/tarantool/issues/5210

A changelog entry is needed here.

I would include information about a range of affected versions (square
bracket comments are for committer):

- from 2.5.0-265-g3af79e70b (inclusive) to [[`git describe` from 2.5 branch]] (exclusive)
- all 2.6.* until [[`git describe` from master branch]] (exclusive)

WBR, Alexander Turenko.

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

[Alexander Turenko]

> 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.

Re: [Tarantool-patches] [PATCH 2/2] test: add regression test for table.clear()

[Alexander Turenko]

> --- /dev/null
> +++ b/test/box-tap/gh-5210-table-clear.test.lua

I would place it into app-tap, because it is not related to box.

> +t = {a = 1, b = 2}

I would use 'local' here.

> +test:is(table.clear(t), nil, 'table clear')

I would check that the table is cleared: say, using `next(t)`. It must
return `nil` for an empty table.

WBR, Alexander Turenko.

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

[Oleg Babin]

Hi! Thanks for your patch!

I think it shouldn't be placed in "src/lua/trigger.lua". I believe 
"src/lua/table.lua" is more appropriate place.

Of course with comment why it should be done e.g. "This require modifies 
global "table" module and adds "clear" function to it".

On 28/07/2020 16:52, sergeyb@tarantool.org wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
>
> Import of 'table.clear' module has been removed
> to fix luacheck warning about unused variable in
> commit 3af79e70b5e1e9b1d69b97f3031a299132a02d2f
> and method table.clear() became unavailable in Tarantool.
>
> Part of #5210
> ---
>   src/lua/trigger.lua | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/src/lua/trigger.lua b/src/lua/trigger.lua
> index 1330ecdd4..066329ea6 100644
> --- a/src/lua/trigger.lua
> +++ b/src/lua/trigger.lua
> @@ -1,4 +1,5 @@
>   local fun = require('fun')
> +local _ = require('table.clear')

BTW, "require('table.clear')" should be enough without "local _ ="

>   
>   --
>   -- Checks that argument is a callable, i.e. a function or a table

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

[Sergey Bronnikov]

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@

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

[Sergey Bronnikov]

Hi, Oleg!
Thanks for review!

On 16:03 Wed 12 Aug , Oleg Babin wrote:
> Hi! Thanks for your patch!
> 
> I think it shouldn't be placed in "src/lua/trigger.lua". I believe
> "src/lua/table.lua" is more appropriate place.

Agree, "src/lua/table.lua" is more appropriate place for it, updated.

> Of course with comment why it should be done e.g. "This require modifies
> global "table" module and adds "clear" function to it".

Done.

> On 28/07/2020 16:52, sergeyb@tarantool.org wrote:
> > From: Sergey Bronnikov <sergeyb@tarantool.org>
> > 
> > Import of 'table.clear' module has been removed
> > to fix luacheck warning about unused variable in
> > commit 3af79e70b5e1e9b1d69b97f3031a299132a02d2f
> > and method table.clear() became unavailable in Tarantool.
> > 
> > Part of #5210
> > ---
> >   src/lua/trigger.lua | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/src/lua/trigger.lua b/src/lua/trigger.lua
> > index 1330ecdd4..066329ea6 100644
> > --- a/src/lua/trigger.lua
> > +++ b/src/lua/trigger.lua
> > @@ -1,4 +1,5 @@
> >   local fun = require('fun')
> > +local _ = require('table.clear')
> 
> BTW, "require('table.clear')" should be enough without "local _ ="

Removed it.

> >   --
> >   -- Checks that argument is a callable, i.e. a function or a table

-- 
sergeyb@

Re: [Tarantool-patches] [PATCH 2/2] test: add regression test for table.clear()

[Sergey Bronnikov]

Alexander, thanks for review!

On 19:41 Tue 28 Jul , Alexander Turenko wrote:
> > --- /dev/null
> > +++ b/test/box-tap/gh-5210-table-clear.test.lua
> 
> I would place it into app-tap, because it is not related to box.

ok, moved to app-tap suite.

> > +t = {a = 1, b = 2}
> 
> I would use 'local' here.

Added.

> > +test:is(table.clear(t), nil, 'table clear')
> 
> I would check that the table is cleared: say, using `next(t)`. It must
> return `nil` for an empty table.

Added is_deeply() to make sure it is empty:

local t = {a = 1, b = 2}
test:is(table.clear(t), nil, 'table clear')
test:is_deeply(t, {}, 'table is clear')

> WBR, Alexander Turenko.

-- 
sergeyb@

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

[Alexander Turenko]

LGTM except one nit (no need to re-review with me):

Please, remove the changelog entry from the commit message. It is for
release notes and should be placed in a cover letter (or under --- for a
singleton patchset). There are cons and pros, but current process is so.

WBR, Alexander Turenko.

Re: [Tarantool-patches] [PATCH 0/2] Recover of table.clear() method

[Sergey Bronnikov]

Patch requires entry in release notes:

@Changelog
Fixed: import of table.clear() method

Affected versions:

- from 2.5.0-265-g3af79e70b (inclusive) to [[`git describe` from 2.5 branch]] (exclusive)
- all 2.6.* until [[`git describe` from master branch]] (exclusive)

On 16:52 Tue 28 Jul , sergeyb@tarantool.org wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
> 
> Patch [1] introduced regression -
> import of 'table.clear' module has been removed. This patch series
> fix a bug and adds a test to clear a table.
> 
> 1. https://github.com/tarantool/tarantool/commit/3af79e70b5e1e9b1d69b97f3031a299132a02d2f
> 
> GitHub issue: https://github.com/tarantool/tarantool/issues/5210
> GitLab pipeline: https://gitlab.com/tarantool/tarantool/-/pipelines/171616899
> 
> Sergey Bronnikov (2):
>   src: return back import of table.clear() method
>   test: add regression test for table.clear()
> 
>  src/lua/trigger.lua                       |  1 +
>  test/box-tap/gh-5210-table-clear.test.lua | 10 ++++++++++
>  2 files changed, 11 insertions(+)
>  create mode 100755 test/box-tap/gh-5210-table-clear.test.lua
> 
> -- 
> 2.26.2
> 

-- 
sergeyb@

Re: [Tarantool-patches] [PATCH 0/2] Recover of table.clear() method

[Kirill Yukhin]

Hello,

On 28 июл 16:52, sergeyb@tarantool.org wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
> 
> Patch [1] introduced regression -
> import of 'table.clear' module has been removed. This patch series
> fix a bug and adds a test to clear a table.
> 
> 1. https://github.com/tarantool/tarantool/commit/3af79e70b5e1e9b1d69b97f3031a299132a02d2f
> 
> GitHub issue: https://github.com/tarantool/tarantool/issues/5210
> GitLab pipeline: https://gitlab.com/tarantool/tarantool/-/pipelines/171616899
> 
> Sergey Bronnikov (2):
>   src: return back import of table.clear() method
>   test: add regression test for table.clear()

I've checked your patch into 2.5 and master.

--
Regards, Kirill Yukhin