From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp58.i.mail.ru (smtp58.i.mail.ru [217.69.128.38]) (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 9C558469719 for ; Mon, 14 Sep 2020 22:02:23 +0300 (MSK) Date: Mon, 14 Sep 2020 22:02:27 +0300 From: Alexander Turenko Message-ID: <20200914190227.w4f5nkgklqz6dx7k@tkn_work_nb> References: <1594375434-743-1-git-send-email-alyapunov@tarantool.org> <1594375434-743-2-git-send-email-alyapunov@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1594375434-743-2-git-send-email-alyapunov@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] Introduce fselect - formatted select List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Aleksandr Lyapunov Cc: tarantool-patches@dev.tarantool.org I have not much to say here. It's nice. It is for humans and so unlikely we should provide some guarantees except options meaning. My first thoughts were how to generalize and allow to reuse it. box.execute() also provides columns metainfo, but the format is a bit different. I guess it'll be the first request to us: support such output in SQL. Maybe just for box.execute(), via Lua. However we can be consider it as a separate task. I also had thoughts whether we should tweak formatting things on some other level: say, provide an option for :select(), which sets a metatable with particular __serialize function. So data will be available as usual in a script, but will be formatted this way in the interactive console. I'm not sure, because there is the donwside: it surely not so short as 'fselect'. I see that you continue to improve the patch on the branch: now 'sql', 'github' and 'jira' delimiters are supported. For me it looks like it goes to be a separate module for formatting tables (like 'csv' we already have). Not just 'formatted select' thing. However here I'm not sure too. That's all. It is nice and it is okay to push from my side. Several nits are below. WBR, Alexander Turenko. > +-- index.fselect - formatted select. > +base_index_mt.fselect = function(index, key, opts) > + -- options. > + local max_width = 140 We can obtain terminal width and use it as default: | tarantool> ffi = require('ffi') | tarantool> ffi.cdef('void rl_get_screen_size(int *rows, int *cols);') | tarantool> colsp = ffi.new('int[1]') | tarantool> ffi.C.rl_get_screen_size(nil, colsp) | tarantool> colsp[0] | --- | - 190 | ... It returns zero, when I run it from a file (not from the console), so a default when there is no terminal size information is necessary anyway. Not sure about portability of calling rl_get_screen_size(), though. > + local num_rows = #tab > + local space = box.space[index.space_id] > + local fmt = space:format() Nit: trailing whitespace. > + -- format string - cut or fill with spaces to make is exactly n symbols. > + -- also replace spaces with non-break spaces. > + local fmt_str = function(x, n) > + if not x then x = '' end > + local str > + if x:len() <= n then > + local add = n - x:len() > + local addl = math.floor(add/2) > + local addr = math.ceil(add/2) > + str = string.rep(' ', addl) .. x .. string.rep(' ', addr) There is quite similar string.center() function (src/lua/string.lua).