[Tarantool-patches] [PATCH v2 2/2] box: fix formatting in session.push
Chris Sosnin
k.sosnin at tarantool.org
Fri Apr 3 15:38:26 MSK 2020
Thank you very much for the review and for your fixes!
I squashed your changes and pushed the updated version.
> On 3 Apr 2020, at 02:14, Vladislav Shpilevoy <v.shpilevoy at tarantool.org> wrote:
>
> 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.
>
>
More information about the Tarantool-patches
mailing list