[tarantool-patches] Re: [PATCH 4/5] box/console: Fix hang in remote console lua mode

[Cyrill Gorcunov <gorcunov@gmail.com>]

On Tue, Sep 03, 2019 at 10:00:02PM +0300, Konstantin Osipov wrote:
> * Cyrill Gorcunov <gorcunov@gmail.com> [19/08/31 00:51]:
> 
> The approach is fine, but I can not stop myself from nitpicking on
> the implementation consistency. Somehow I find this code rather hard to
> read, I tried to find out why, please see below.

This is perfectly fine, I send the series to gather comments and
fix the nits, so I really appreciate it!

> 
> >  
> > +local function current_eos()
> 
> No comment for the function.

ok

> 
> > +    local d = current_output()
> > +    return output_eos[d["fmt"]]
> 
> Why do you need a local variable? 

to make code shorter, the line

	return output_eos[current_output()["fmt"]]

looks too heavy for me. But I must confess I always
forget that we're in lua, where no optimization
would happen (I think) and it will cause jit
to allocate a variable first.

On the other hand the lua handling here should
not be a hot path and make code a bit slower
for beauty/readability sake looks like acceptable
trade off?

> > +end
> 
> Actually, the function is so simple, it is questionable why you
> need it at all. Can't you keep the value pre-computed in the
> object or in the metatable?

Since eos may be changed on the fly I think we can't
precompute it and keep somewhere.

> > +-- Map output format into a "\set" command.
> > +local function output_cmd_string()
> > +    local d = current_output()
> > +    if d["fmt"] == "yaml" then
> > +        return "\\set output yaml"
> > +    elseif d["fmt"] == "lua" then
> > +        local cmd = "\\set output lua"
> > +        if d["opts"] == "block" then
> > +            cmd = cmd .. ",block"
> > +        end
> > +        return cmd
> > +    end
> > +    return ""
> > +end
> 
> You could use string.format here:
> string.format("\\set output %s", format)

Will try to use, thanks!

> 
> The code would win if you pass in 'd' as an argument, rather than
> compute it locally, and give it a meaningful name.
> 
> Ideally, why not store the whole thing in self and compute once.

But the eos is per session value, can be changed dynamically.
Or you mean pre-compute this command for each output once
and just fetch it from the precomputed set?

> 
> > +        -- The command might modify EOS so
> > +        -- we have to parse and preprocess it
> > +        -- first to not stuck in :read() forever.
> > +        preprocess_eval = function(self, text)
> > +            local command = get_command(text)
> 
> Why get_command() but not get_current_eos()?

Because it generates \set output command, not eos
as a symbol. Probably better name it gen_eos_command.

> 
> > +            if command == nil then
> > +                return
> > +            end
> > +            local nr_items, items = parse_operators(command)
> > +            if nr_items == 3 then
> > +                local err, fmt, opts = output_parse(items[3])
> 
> why parse_operators but output_parse? 

Because I try to use output_ as a prefix to "output" engine,
ie everything related to output would have this prefix. Easier
for grep.

> > +                if not err then
> > +                    self.eos = output_eos[fmt]
> 
> why self.eos but global output_eos? 

because this is not global output but the value bound
to a particular session, you can have one server and
several clients, where each client have own eos
setting. Same applies to next your comment. So that
why we need to carry it into object instance.

> 
> 
> > +                end
> > +            end
> > +        end,
> 
> Why is this function part of the metatable? Can not it be a
> standalone function, not exported in the metatable?
> 
> > +    local cmd_set_output = output_cmd_string() .. '\n'
> > +    local cmd_delimiter = 'require("console").delimiter("$EOF$")\n'
> > +    if not conn:write(cmd_set_output) or not conn:read() then
> > +        conn:set_error()
> > +    end
> > +    if not conn:write(cmd_delimiter) or not conn:read() then
> >          conn:set_error()
> 
> Please add a comment for what's going on here.

OK