[tarantool-patches] Re: [PATCH v1 1/1] Tarantoolctl "enter" with set language

[Imeev Mergen <imeevma@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 8667 bytes --]



On 07/26/2018 12:56 AM, Vladislav Shpilevoy wrote:
>
>
> 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.
Done.
>
> 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.
Done.
>
>> +    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.
Done.
>
>> +        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.
Discussed, partly changed.
>
>> +
>> +    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.
Discussed, decided to reject the idea.
>
> 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.
>
Done.

commit 7088606bba3810ffc6db313cf31d7aa101d7175d
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Fri Jul 13 19:25:32 2018 +0300

     Tarantoolctl "enter" with set language

     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.

diff --git a/extra/dist/CMakeLists.txt b/extra/dist/CMakeLists.txt
index 862689e..3045b5a 100644
--- a/extra/dist/CMakeLists.txt
+++ b/extra/dist/CMakeLists.txt
@@ -34,6 +34,7 @@ message (STATUS "tarantoolctl logdir: 
${TARANTOOL_LOGDIR}")
  set(TARANTOOL_RUNDIR "${CMAKE_INSTALL_FULL_LOCALSTATEDIR}/run/tarantool")
  message (STATUS "tarantoolctl rundir: ${TARANTOOL_RUNDIR}")
  set(TARANTOOL_USER "tarantool")
+set(TARANTOOL_LANGUAGE "lua")
  set(SYSCONFIG_AVAILABLEDIR "tarantool/instances.available")
  set(SYSCONFIG_ENABLEDDIR "tarantool/instances.enabled")
  set(TARANTOOL_AVAILABLEDIR 
"${CMAKE_INSTALL_FULL_SYSCONFDIR}/${SYSCONFIG_AVAILABLEDIR}")
diff --git a/extra/dist/default/tarantool.in 
b/extra/dist/default/tarantool.in
index 9a8dad3..01e7144 100644
--- a/extra/dist/default/tarantool.in
+++ b/extra/dist/default/tarantool.in
@@ -20,6 +20,7 @@ default_cfg = {
      vinyl_dir  = "@TARANTOOL_DATADIR@", -- 
@TARANTOOL_DATADIR@/${INSTANCE}
      log        = "@TARANTOOL_LOGDIR@", -- 
@TARANTOOL_LOGDIR@/${INSTANCE}.log
      username   = "@TARANTOOL_USER@",
+    language   = "@TARANTOOL_LANGUAGE@",
  }

  -- instances.available - all available instances
diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in
index 2dd5d74..ef9da2f 100755
--- a/extra/dist/tarantoolctl.in
+++ b/extra/dist/tarantoolctl.in
@@ -47,6 +47,7 @@ local default_cfg
  local positional_arguments
  local keyword_arguments
  local lua_arguments = arg
+local language

  -- function for printing usage reference
  local usage
@@ -155,6 +156,14 @@ 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')

+    language = (d.language or "lua"):lower()
+    d.language  = nil
+
+    if language ~= "lua" and language ~= "sql" then
+        log.error('Unknown language: %s', language)
+        os.exit(1)
+    end
+
      default_cfg = d

      if not usermode then
@@ -629,6 +638,12 @@ local function logrotate()
  end

  local function enter()
+    local options = keyword_arguments
+    language = (options.language or language):lower()
+    if language ~= "lua" and language ~= "sql" then
+        log.error('Unknown language: %s', options.language)
+        return 1
+    end
      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 +659,9 @@ local function enter()
          console_sock, TIMEOUT_INFINITY
      )

-    console.on_start(function(self) self:eval(cmd) end)
+    local cmd_connect = string.format("\\set language %s", language)
+
+    console.on_start(function(self) self:eval(cmd) 
self:eval(cmd_connect) end)
      console.on_client_disconnect(function(self) self.running = false end)
      console.start()
      return 0
@@ -1008,11 +1025,15 @@ local commands = setmetatable({
          }
      }, enter = {
          func = exit_wrapper(enter), process = process_local, help = {
-            header = "%s enter INSTANCE",
+            header = "%s enter INSTANCE [--language=language]",
              linkmode = "%s enter",
              description =
  [=[
-        Enter an instance's interactive Lua console.
+        Enter an instance's interactive Lua or SQL console.
+
+        Supported options:
+        * --language=language to set interactive console language.
+          May be either Lua or SQL.
  ]=],
              weight = 65,
              deprecated = false,
@@ -1284,6 +1305,7 @@ do
          { 'chdir',       'string'  },
          { 'only-server', 'string'  },
          { 'server',      'string'  },
+        { 'language',    'string'  },
      })

      local cmd_name


[-- Attachment #2: Type: text/html, Size: 14072 bytes --]