From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp41.i.mail.ru (smtp41.i.mail.ru [94.100.177.101]) (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 35A664696C3 for ; Fri, 3 Apr 2020 15:38:28 +0300 (MSK) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 13.0 \(3608.60.0.2.5\)) From: Chris Sosnin In-Reply-To: Date: Fri, 3 Apr 2020 15:38:26 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <04FDF4FC-F690-446D-8C7C-11499D8D05D0@tarantool.org> References: <33ee0fa0264869e7f5c0a952b2812a1125e56a82.1584110394.git.k.sosnin@tarantool.org> <7157885B-8F81-44C4-A317-1DA71CFC8008@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2 2/2] box: fix formatting in session.push List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org 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 = wrote: >=20 > Hi! Thanks for the fixes! >=20 >>>> + lua_pushvalue(L, -3); >>>> + lua_pcall(L, 1, 1, 0); >>>=20 >>> 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. >>=20 >> I couldn=E2=80=99t really find a way to make serpent raise an error. >=20 > 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. >=20 >>>=20 >>>> } >>>> - assert(rc =3D=3D 1); >>>> assert(lua_isstring(L, -1)); >>>> size_t len; >>>> const char *result =3D 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 >>>>=20 >>>> +-- Used by console_session_push. >>>> +box_internal.format_lua_value =3D function(value) >>>> + local opts =3D current_output()["opts"] >>>> + value =3D format_lua_value(value, opts) >>>> + return '-- Push begin\n' .. value .. '\n-- Push end\n' >>>=20 >>> 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. >>=20 >> I agree, removed -- Push end=20 >=20 > Since there is no 'Push end' anymore, it should not be > called 'Push begin'. I renamed it to just 'Push'. >=20 > Also you didn't add a test on the push arriving out of > bound. I did it on the branch. >=20 > 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. >=20 >=20