From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id ED7E520C55 for ; Wed, 4 Sep 2019 10:53:48 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id x7F6QAIgB-i9 for ; Wed, 4 Sep 2019 10:53:48 -0400 (EDT) Received: from mail-lj1-f173.google.com (mail-lj1-f173.google.com [209.85.208.173]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 80E3520C4B for ; Wed, 4 Sep 2019 10:53:48 -0400 (EDT) Received: by mail-lj1-f173.google.com with SMTP id u14so13359892ljj.11 for ; Wed, 04 Sep 2019 07:53:48 -0700 (PDT) Date: Tue, 3 Sep 2019 22:18:13 +0300 From: Cyrill Gorcunov Subject: [tarantool-patches] Re: [PATCH 4/5] box/console: Fix hang in remote console lua mode Message-ID: <20190903191813.GL2801@uranus.lan> References: <20190830214808.17422-1-gorcunov@gmail.com> <20190830214808.17422-5-gorcunov@gmail.com> <20190903190002.GC15611@atlas> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190903190002.GC15611@atlas> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: Konstantin Osipov Cc: tml , Alexander Turenko , Kirill Yukhin On Tue, Sep 03, 2019 at 10:00:02PM +0300, Konstantin Osipov wrote: > * Cyrill Gorcunov [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