[tarantool-patches] Re: [PATCH v2 1/1] tuple:tomap: only_names option

["v.shpilevoy@tarantool.org" <v.shpilevoy@tarantool.org>]

See below 5 comments.

> 27 марта 2018 г., в 13:06, Kirill Shcherbatov <kshcherbatov@tarantool.org> написал(а):
> 
> Fixes #3280
> ---
> src/box/lua/tuple.c        | 23 ++++++++++++++++++++---
> test/engine/tuple.result   | 45 +++++++++++++++++++++++++++++++++++++++++++++
> test/engine/tuple.test.lua | 14 ++++++++++++++
> 3 files changed, 79 insertions(+), 3 deletions(-)
> 
> diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
> index d5ed7c4..da2a80b 100644
> --- a/src/box/lua/tuple.c
> +++ b/src/box/lua/tuple.c
> @@ -280,9 +280,21 @@ luamp_encode_extension_box(struct lua_State *L, int idx,
> static int
> lbox_tuple_to_map(struct lua_State *L)
> {
> -	if (lua_gettop(L) < 1)
> -		luaL_error(L, "Usage: tuple:tomap()");
> -	const struct tuple *tuple = lua_checktuple(L, 1);
> +    int argc = lua_gettop(L);
> +	if (argc < 1 || argc > 2)
> +		goto error;
> +	int names_only = 0;
> +	if (argc == 2) {
> +		if (!lua_istable(L, 2))
> +			goto error;
> +		lua_getfield(L, 2, "names_only");
> +		if (!lua_isboolean(L, -1))
> +			goto error;
> +		names_only = lua_toboolean(L, -1);
> +		lua_pop(L, 1);

1. Seems that you fixed only indentation. Please, fix other comments too.

> +    }
> +
> +    const struct tuple *tuple = lua_checktuple(L, 1);
> 	const struct tuple_format *format = tuple_format(tuple);
> 	const struct tuple_field *field = &format->fields[0];
> 	const char *pos = tuple_data(tuple);
> @@ -294,6 +306,8 @@ lbox_tuple_to_map(struct lua_State *L)
> 		lua_pushstring(L, field->name);
> 		luamp_decode(L, luaL_msgpack_default, &pos);
> 		lua_rawset(L, -3);
> +		if (names_only)
> +			continue;
> 		/*
> 		 * Access the same field by an index. There is no
> 		 * copy for tables - lua optimizes it and uses
> @@ -309,6 +323,9 @@ lbox_tuple_to_map(struct lua_State *L)
> 		lua_rawseti(L, -2, i + TUPLE_INDEX_BASE);
> 	}
> 	return 1;
> +error:
> +	luaL_error(L, "Usage: tuple:tomap() or tuple:tomap({names_only = true})");
> +	return 1;
> }
> 
> /**
> 
> diff --git a/test/engine/tuple.test.lua b/test/engine/tuple.test.lua
> index 6d7d254..0f139a3 100644
> --- a/test/engine/tuple.test.lua
> +++ b/test/engine/tuple.test.lua
> @@ -200,5 +200,19 @@ t1map = t1:tomap()
> maplen(t1map), t1map[1], t1map[2], t1map[3]
> s:drop()
> 
> +--
> +-- gh-2821: tuple:tomap() names_only feature.
> +--
> +subscriber = box.schema.space.create('subscriber')
> +box.schema.user.grant('guest','read,write,execute','space', 'subscriber')

2. You do not need rights, when work locally.

> +subscriber:format({{'contract_id', 'integer'}, {'subscriber_id', 'integer'}, {'phone', 'string'}, {'subscriber_status_id', 'integer'}, })
> +idx = subscriber:create_index('primary', {parts={'contract_id', 'subscriber_id'}})
> +--subscriber:create_index('phone', {parts={'phone'}, unique = false })

3. Please, do not put a code in comment. If you do not need it, then just delete.

> +subscriber:replace({1, 1, '79160000000', 42})
> +t = subscriber:get({1, 1})

4. replace() returns a result tuple, so you can just do t = subscriber:replace(), with no get().

> +t:tomap({names_only = false})
> +t:tomap({names_only = true})

5. Like I wrote in the previous review, please, add tests on tomap with invalid options, on tomap with names_only = non-boolean, on tomap with empty options. Do not hurry so much.

> +s:drop()
> +
> engine = nil
> test_run = nil
> -- 
> 2.7.4
> 
>