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 01A1A22839 for ; Wed, 25 Jul 2018 17:56:35 -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 8Z0OpkWAIsWH for ; Wed, 25 Jul 2018 17:56:34 -0400 (EDT) Received: from smtp44.i.mail.ru (smtp44.i.mail.ru [94.100.177.104]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 3EE15200BB for ; Wed, 25 Jul 2018 17:56:34 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v1 1/1] Tarantoolctl "enter" with set language References: From: Vladislav Shpilevoy Message-ID: Date: Thu, 26 Jul 2018 00:56:30 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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: tarantool-patches@freelists.org, imeevma@tarantool.org On 25/07/2018 10:51, imeevma@tarantool.org wrote: > This patch allow to use option "--language" with > "tarantoolctl enter" command. Also default value > for language were added in default configuration > file. Language is either SQL or Lua. > > Closes #2385. > --- > Branch: https://github.com/tarantool/tarantool/tree/imeevma/gh-2385-select-input-language-tarantoolctl > Issue: https://github.com/tarantool/tarantool/issues/2385 > > diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in > index 2dd5d74..55711b8 100755 > --- a/extra/dist/tarantoolctl.in > +++ b/extra/dist/tarantoolctl.in > @@ -155,6 +155,11 @@ local function load_default_file(default_file) > d.vinyl_dir = fio.pathjoin(d.vinyl_dir, instance_name) > d.log = fio.pathjoin(d.log, instance_name .. '.log') > > + d.language = (d.language or "lua"):lower() > + if d.language ~= "lua" and d.language ~= "sql" then > + d.language = "lua" 1. Please, do not ignore unknown language. Log this error like others in the same function already do. 2. Why do you need language in default_cfg? It is box.cfg as I understand and box.cfg has no language option. It is not? If you need to parse a language from the file, then you can store it in another table or even in a separate variable like others on lines 30-49. > + end > + > default_cfg = d > > if not usermode the> @@ -629,6 +635,17 @@ local function logrotate() > end > > local function enter() > + local options = keyword_arguments > + local language = options.language > + if language ~= nil then > + language = language:lower() > + if language ~= "lua" and language ~= "sql" then > + log.warn("Language \"%s\" is not recognized. Default language" .. > + " \"%s\" is set.", language, default_cfg.language) > + language = nil 3. For enter() function it is ok to fail on unknown language. You should not ignore the error because a caller could use something like this: cat my_script.sql | tarantoolctl enter --language sqlll And if you ignore invalid language, this command will feed the entire script on SQL into Lua console. It will produce huge number of syntax errors if no worse. > + end > + end > + language = language or default_cfg.language > local console_sock_path = uri.parse(console_sock).service > if fio.stat(console_sock_path) == nil then > log.error("Can't connect to %s (%s)", console_sock_path, errno.strerror()) > @@ -644,7 +661,9 @@ local function enter() > console_sock, TIMEOUT_INFINITY > ) > > - console.on_start(function(self) self:eval(cmd) end) > + local cmd_language = string.format("\\set language %s", language) 4. Why not to merge cmd_language into cmd? It is created a line above. > + > + console.on_start(function(self) self:eval(cmd) self:eval(cmd_language) end) > console.on_client_disconnect(function(self) self.running = false end) > console.start() > return 0 > @@ -1284,6 +1303,7 @@ do > { 'chdir', 'string' }, > { 'only-server', 'string' }, > { 'server', 'string' }, > + { 'language', 'string' }, > }) > > local cmd_name > 5. Lets add the same option to 'eval', 'connect' to make the commands symmetric. 6. Please, update the comments. For example, on line 1028 I still see the comment about 'enter': "Enter an instance's interactive Lua console", but actually it is not complete now - you can choose SQL.