From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <v.shpilevoy@tarantool.org>
Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251])
 (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 292ED4696C3
 for <tarantool-patches@dev.tarantool.org>;
 Fri,  3 Apr 2020 02:14:18 +0300 (MSK)
References: <cover.1584110394.git.k.sosnin@tarantool.org>
 <33ee0fa0264869e7f5c0a952b2812a1125e56a82.1584110394.git.k.sosnin@tarantool.org>
 <d0e61613-9f78-6526-0d18-367a6b0c19ce@tarantool.org>
 <7157885B-8F81-44C4-A317-1DA71CFC8008@tarantool.org>
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Message-ID: <e3a8f86c-bcde-9276-8082-32583cb3ddb7@tarantool.org>
Date: Fri, 3 Apr 2020 01:14:16 +0200
MIME-Version: 1.0
In-Reply-To: <7157885B-8F81-44C4-A317-1DA71CFC8008@tarantool.org>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Subject: Re: [Tarantool-patches] [PATCH v2 2/2] box: fix formatting in
	session.push
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Chris Sosnin <k.sosnin@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org

Hi! Thanks for the fixes!

>>> +		lua_pushvalue(L, -3);
>>> +		lua_pcall(L, 1, 1, 0);
>>
>> 2. In case lua_pcall() fails, the function should fail too, and should
>> return NULL + set an error in diag. Also it would be good to have a
>> test on it.
> 
> I couldn’t really find a way to make serpent raise an error.

Yeah, me neither. But anyway an error should be handled. With
panic or with return -1, but we won't be able to test it.

>>
>>> 	}
>>> -	assert(rc == 1);
>>> 	assert(lua_isstring(L, -1));
>>> 	size_t len;
>>> 	const char *result = lua_tolstring(L, -1, &len);
>>> diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
>>> index 7208d3d30..fb7d9ca9b 100644
>>> --- a/src/box/lua/console.lua
>>> +++ b/src/box/lua/console.lua
>>> @@ -207,6 +208,18 @@ local function current_output()
>>>     end
>>> end
>>>
>>> +-- Used by console_session_push.
>>> +box_internal.format_lua_value = function(value)
>>> +    local opts = current_output()["opts"]
>>> +    value = format_lua_value(value, opts)
>>> +    return '-- Push begin\n' .. value .. '\n-- Push end\n'
>>
>> 3. Did you check that it is really necessary to append
>> a special 'push end' tail? I couldn't find any Lua value,
>> which couldn't be identified by 'push begin' and eos (';')
>> markers.
> 
> I agree, removed -- Push end 

Since there is no 'Push end' anymore, it should not be
called 'Push begin'. I renamed it to just 'Push'.

Also you didn't add a test on the push arriving out of
bound. I did it on the branch.

See my review fixes on top of this commit on the branch,
and below. If you agree, squash them. If don't, then lets
discuss.