[Tarantool-patches] [PATCH] Add console.print() alias for box.session.push()

[Tarantool-patches] [PATCH] Add console.print() alias for box.session.push()

[Sergey Voinov]

Currently, lack of a function like console.print() often confuses users.
This change adds such alias.

Closes: #4393
---
 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)
+    if message == nil then
+        error('Usage: console.print(message, sync)')
+    end
+    return box.session.push(message, sync or box.session.sync())
+end
+
 package.loaded['console'] = {
     start = start;
     eval = eval;
@@ -834,4 +844,5 @@ package.loaded['console'] = {
     on_start = on_start;
     on_client_disconnect = on_client_disconnect;
     completion_handler = internal.completion_handler;
+    print = print;
 }
diff --git a/test/app-tap/console.test.lua b/test/app-tap/console.test.lua
index da5c1e71e..e8a4c39d0 100755
--- a/test/app-tap/console.test.lua
+++ b/test/app-tap/console.test.lua
@@ -21,7 +21,7 @@ local EOL = "\n...\n"
 
 test = tap.test("console")
 
-test:plan(73)
+test:plan(78)
 
 -- Start console and connect to it
 local server = console.listen(CONSOLE_SOCKET)
@@ -306,6 +306,29 @@ local res = yaml.decode(client:read(EOL))[1]
 test:is_deeply(res, exp_res, 'unknown command')
 client:close()
 
+--
+-- gh-4393: console.print() alias for box.session.push()
+--
+client = socket.tcp_connect("unix/", CONSOLE_SOCKET)
+
+-- console.print, no arguments.
+client:write('console.print()\n')
+test:isnt(string.match(client:read(EOL), 'Usage: console%.print%(message, sync%)'), nil,
+        "error message")
+
+-- console.print, one argument.
+client:write('console.print(200)\n')
+test:is(client:read(EOL), '%TAG !push! tag:tarantool.io/push,2018\n--- 200\n...\n',
+        "pushed message")
+test:is(client:read(EOL), '---\n- true\n...\n', "pushed message")
+
+-- 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()
+
 server:close()
 
 box.schema.user.drop('test')
-- 
2.17.1

Re: [Tarantool-patches] [PATCH] Add console.print() alias for box.session.push()

[Cyrill Gorcunov]

On Thu, Dec 05, 2019 at 06:53:06PM +0300, Sergey Voinov wrote:
> Currently, lack of a function like console.print() often confuses users.
> This change adds such alias.
> 
> Closes: #4393
> ---
>  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)
> +    if message == nil then
> +        error('Usage: console.print(message, sync)')
> +    end
> +    return box.session.push(message, sync or box.session.sync())
> +end

Could you plase rename it to console_print() or similar since
bare print() is already overloaded too much.

Re: [Tarantool-patches] [PATCH] Add console.print() alias for box.session.push()

[Alexander Turenko]

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.)

Re: [Tarantool-patches] [PATCH] Add console.print() alias for box.session.push()

[Alexander Turenko]

> > +--
> > +-- Alias for box.session.push
> > +--
> > +local function print(message, sync)
> 
> <...>
>
> '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).

FYI: I filed https://github.com/tarantool/tarantool/issues/4689 to
remove `sync` argument from box.sesison.push().

WBR, Alexander Turenko.