[PATCH 6/7] box/console: Provide console.eos() api

Konstantin Osipov kostja.osipov at gmail.com
Tue Oct 1 23:19:21 MSK 2019


* Cyrill Gorcunov <gorcunov at gmail.com> [19/09/20 21:47]:
> The other modules would be able to find out which eos marker
> is currently active. For example when reading replies from
> remote server via text based console protocol.
> 
> @TarantoolBot document
> Title: > require('console').eos()
> 
> Returns a string with currently active end of string marker.
> 
> @TarantoolBot document
> Title: > require('console').eos("")
> 
> Sets end of string marker for appliable output modes.
> YAML output mode does not support eos change. For Lua
> mode it is possible to set eos to empty string (like
> in example above) or any other symbol.
> 
> Note that default semicolon symbol is mandatory for
> remote text consoles thus if get zapped the read
> procedure from remote socket will not be possible and
> connection get stuck forever. Use this ability with
> extreme caution.

I would not document the ability to set the eos marker. 
Please feel free to keep it for testing, otherwise, if you're not
using it for testing, I would remove it altogether.

This feature only works half-way if we have a chain of console
connections, and we should not bind ourselves with documenting it.


> +    -- We can't allow to change yaml eos format because
> +    -- it is a part of encoding standart, thus permit
> +    -- only for modes where it is safe.
> +    local d = current_output()
> +    if d["fmt"] == "lua" then
> +        output_eos["lua"] = eos_value
> +    else
> +        error("console.eos(): is immutable for output " .. d["fmt"])
> +    end

The comment does not match the implementation. You speak about
yaml, but check for Lua. Shouldn't you check for yaml and give
error, and otherwise allow changing the eos? Otherwise, please fix
the comment stating that changing eos should only work for lua.

Otherwise lgtm.

-- 
Konstantin Osipov, Moscow, Russia




More information about the Tarantool-patches mailing list