From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp46.i.mail.ru (smtp46.i.mail.ru [94.100.177.106]) (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 2969D46970E for ; Tue, 17 Dec 2019 02:54:19 +0300 (MSK) Date: Tue, 17 Dec 2019 02:54:15 +0300 From: Alexander Turenko Message-ID: <20191216235413.knmpld2poafxhg7m@tkn_work_nb> References: <20191205155306.13157-1-sergeiv@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20191205155306.13157-1-sergeiv@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] Add console.print() alias for box.session.push() List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Voinov Cc: tarantool-patches@dev.tarantool.org I was unsure whether we should print a YAML tag: it allows to distinguish pushes from responses and necessary for automatic processing of a console output, however looks redundant for interactive usage. The same situation for statements delimiter in lua output: I don't think that an end user should see them. I'll file separate issue about it. My key points: * A remote console may be configured by a client. * A client may lean on isatty(). * A client may postprocess remote console output. * A client also should parse passed commands to adjust postprocessor accordingly. Anyway, it is out of scope here. I think now we should stick to the most general solution: both for tools and end users. So it seems logical to show yaml tags for pushes. See other comments below. WBR, Alexander Turenko. On Thu, Dec 05, 2019 at 06:53:06PM +0300, Sergey Voinov wrote: > Add console.print() alias for box.session.push() > > Currently, lack of a function like console.print() often confuses users. > This change adds such alias. > > Closes: #4393 Please, add a [docbot](https://github.com/tarantool/docbot) comment to request console reference change from our documentation team. See examples in git log. Describe what is changed from user's perspective. Give hints how it is intended to use (for debugging). It will be likely added to [1], so both formal API and informal description will be useful for our documentation team. [1]: https://www.tarantool.io/en/doc/1.10/book/box/box_session/ > --- > issue: https://github.com/tarantool/tarantool/issues/4393 > branch: https://github.com/tarantool/tarantool/compare/servoin/gh-4393-console_print > src/box/lua/console.lua | 11 +++++++++++ > test/app-tap/console.test.lua | 25 ++++++++++++++++++++++++- > 2 files changed, 35 insertions(+), 1 deletion(-) > > diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua > index d4d8ec984..05f8016c4 100644 > --- a/src/box/lua/console.lua > +++ b/src/box/lua/console.lua > @@ -821,6 +821,16 @@ local function listen(uri) > return s > end > > +-- > +-- Alias for box.session.push > +-- > +local function print(message, sync) I vote up Cyrill's comment: let's rename it to console_print (I mean, local function name; console.print() is okay). 'sync' is not used here, see console_session_push() comment: | /** | * Push a tagged YAML document into a console socket. | * @param session Console session. | * @param sync Unused request sync. | * @param port Port with YAML to push. | * | * @retval 0 Success. | * @retval -1 Error. | */ | static int | console_session_push(struct session *session, uint64_t sync, struct port *port) Your last test case shows it: we pass some random 'sync', but anyway receive a push from a console. The parameter exists, because box.session.push() API is general mechanism. It just ignored for 'repl' and 'console' sessions (see [2]). Since console.print() is console specific rather then general I don't see a reason to support 'sync' argument here. Moreover I would restrict it for 'repl' and 'console' sessions: let's make it no-op otherwise (because debug prints should not raise an error IMHO). I would also receive variable amount of arguments and send all them as pushes, because of two reasons: * Usual print() prints all its arguments. The same for log.info(). * It is convenient for debugging. print() and log.info() print one record for multiple arguments, so maybe it worth to send one push with a list of passed arguments. What about something like the following? | local function console_print(...) | local session_type = box.session.type() | if session_type == 'repl' or session_type == 'console' then | box.session.push(setmetatable({'console.print', ...}, | {__serialize = 'seq'})) | end | end `__serializer = 'seq'` enables 'flow style' for yaml serializer (instead of 'block style'), this encodes the list in one line. [2]: https://www.tarantool.io/en/doc/1.10/book/box/box_session/#box-session-type > + if message == nil then > + error('Usage: console.print(message, sync)') > + end > + return box.session.push(message, sync or box.session.sync()) > +end Usual print() does not return any value. I would not return anything here too (it return `true` now). > +-- console.print, two arguments. > +client:write('console.print(1, 9223372036854775808ULL)\n') > +test:is(client:read(EOL), '%TAG !push! tag:tarantool.io/push,2018\n--- 1\n...\n', > + "pushed message") > +test:is(client:read(EOL), '---\n- true\n...\n', "pushed message") > +client:close() > + (I said about this test case above.)