From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <vdavydov.dev@gmail.com>
Date: Tue, 11 Jun 2019 11:15:06 +0300
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Subject: Re: [PATCH v1 1/2] netbox: store method_encoder args in request
Message-ID: <20190611081506.6b3jftczzd4kmhrl@esperanza>
References: <cover.1560160282.git.imeevma@gmail.com>
 <048ffe41499516502f00d8576d647f50d8596d66.1560160282.git.imeevma@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <048ffe41499516502f00d8576d647f50d8596d66.1560160282.git.imeevma@gmail.com>
To: imeevma@tarantool.org
Cc: tarantool-patches@freelists.org
List-ID: <tarantool-patches.dev.tarantool.org>

On Mon, Jun 10, 2019 at 01:02:23PM +0300, imeevma@tarantool.org wrote:
> This patch changes the way arguments are passed to functions in
> the array method_encoder, and saves these arguments in REQUEST.
> This is necessary to establish a connection between netbox space
> objects and the tuples obtained through the netbox
> 
> Needed for #2978
> ---
>  src/box/lua/net_box.lua   | 179 +++++++++++++++++++++++++++++++---------------
>  test/box/access.result    |  15 +++-
>  test/box/access.test.lua  |   9 ++-
>  test/box/net.box.result   |   6 +-
>  test/box/net.box.test.lua |   6 +-
>  5 files changed, 146 insertions(+), 69 deletions(-)
> 
> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> index 251ad40..8d42fb4 100644
> --- a/src/box/lua/net_box.lua
> +++ b/src/box/lua/net_box.lua
> @@ -25,7 +25,6 @@ local check_primary_index = box.internal.check_primary_index
>  
>  local communicate     = internal.communicate
>  local encode_auth     = internal.encode_auth
> -local encode_select   = internal.encode_select
>  local decode_greeting = internal.decode_greeting
>  
>  local TIMEOUT_INFINITY = 500 * 365 * 86400
> @@ -84,22 +83,70 @@ local function decode_push(raw_data)
>      return response[IPROTO_DATA_KEY][1], raw_end
>  end
>  
> +local function encode_call(send_buf, id, method_args)
> +    return internal.encode_call(send_buf, id, method_args.func_name,
> +                                method_args.args)
> +end
> +local function encode_call_16(send_buf, id, method_args)
> +    return internal.encode_call_16(send_buf, id, method_args.func_name,
> +                                   method_args.args)
> +end
> +local function encode_ping(send_buf, id, ...)
> +    return internal.encode_ping(send_buf, id, ...)
> +end
> +local function encode_eval(send_buf, id, method_args)
> +    return internal.encode_eval(send_buf, id, method_args.code,
> +                                method_args.args)
> +end
> +local function encode_insert(send_buf, id, method_args)
> +    return internal.encode_insert(send_buf, id, method_args.space_id,
> +                                  method_args.tuple)
> +end
> +local function encode_replace(send_buf, id, method_args)
> +    return internal.encode_replace(send_buf, id, method_args.space_id,
> +                                   method_args.tuple)
> +end
> +local function encode_delete(send_buf, id, method_args)
> +    return internal.encode_delete(send_buf, id, method_args.space_id,
> +                                  method_args.index_id, method_args.key)
> +end
> +local function encode_update(send_buf, id, method_args)
> +    return internal.encode_update(send_buf, id, method_args.space_id,
> +                                  method_args.index_id, method_args.key,
> +                                  method_args.oplist)
> +end
> +local function encode_upsert(send_buf, id, method_args)
> +    return internal.encode_upsert(send_buf, id, method_args.space_id,
> +                                  method_args.key, method_args.oplist)
> +end
> +local function encode_select(send_buf, id, method_args)
> +    return internal.encode_select(send_buf, id, method_args.space_id,
> +                                  method_args.index_id, method_args.iterator,
> +                                  method_args.offset, method_args.limit,
> +                                  method_args.key)
> +end
> +local function encode_execute(send_buf, id, method_args)
> +    return internal.encode_execute(send_buf, id, method_args.query,
> +                                   method_args.parameters, method_args.sql_opts)
> +end

Why not call it simply 'args'?

> @@ -1150,14 +1201,16 @@ end
>  -- @deprecated since 1.7.4
>  function remote_methods:eval_16(code, ...)
>      check_remote_arg(self, 'eval')
> -    return unpack((self:_request('eval', nil, code, {...})))
> +    local method_args = {code=code, args={...}}
> +    return unpack((self:_request('eval', nil, method_args)))
>  end
>  
>  function remote_methods:eval(code, args, opts)
>      check_remote_arg(self, 'eval')
>      check_eval_args(args)
>      args = args or {}
> -    local res = self:_request('eval', opts, code, args)
> +    local method_args = {code=code, args=args}

Coding style: we add a space before and after '=':

	local method_args = {code = code, args = args}

Please fix here and everywhere else.

Other than that this patch looks okay to me.