Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH v2 0/2] console: support of backslash
@ 2020-06-10  9:43 Olga Arkhangelskaia
  2020-06-10  9:43 ` [Tarantool-patches] [PATCH 1/2] test: add libisatty to test local console Olga Arkhangelskaia
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Olga Arkhangelskaia @ 2020-06-10  9:43 UTC (permalink / raw)
  To: tarantool-patches; +Cc: alexander.turenko

@ChangeLog
While using local console one can use '\' to carry the line without
setting the delimiter.
Works only with local console.

Changes v2:
Added test case

Olga (1):
  test: add libisatty to test local console

Olga Arkhangelskaia (1):
  console: add line carrying backslash

 src/box/lua/console.lua       | 18 ++++++++-----
 test/CMakeLists.txt           |  8 ++++++
 test/app-tap/gh-4317.test.lua | 51 +++++++++++++++++++++++++++++++++++
 test/isatty.c                 |  5 ++++
 4 files changed, 76 insertions(+), 6 deletions(-)
 create mode 100755 test/app-tap/gh-4317.test.lua
 create mode 100644 test/isatty.c

-- 
2.20.1 (Apple Git-117)

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Tarantool-patches] [PATCH 1/2] test: add libisatty to test local console
  2020-06-10  9:43 [Tarantool-patches] [PATCH v2 0/2] console: support of backslash Olga Arkhangelskaia
@ 2020-06-10  9:43 ` Olga Arkhangelskaia
  2020-06-11 16:47   ` Igor Munkin
  2020-06-10  9:43 ` [Tarantool-patches] [PATCH 2/2] console: add line carrying backslash Olga Arkhangelskaia
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Olga Arkhangelskaia @ 2020-06-10  9:43 UTC (permalink / raw)
  To: tarantool-patches; +Cc: alexander.turenko

From: Olga <arkholga@tarantool.org>

Local console is impossible to test within pipe, tatantool -i.
isatty lib was added to overcome isatty check and test local console.
Hack by Alexandr Turenko.
---
 test/CMakeLists.txt | 8 ++++++++
 test/isatty.c       | 5 +++++
 2 files changed, 13 insertions(+)
 create mode 100644 test/isatty.c

diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 9b5df7dc5..7894e1651 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -67,3 +67,11 @@ endif()
 
 # Disable connector_c for 1.6
 #add_subdirectory(connector_c)
+
+# Build a library with isatty() that always returns 1.
+#
+# It is needed to test the local tarantool console. Consider an
+# example:
+#
+# echo 42 | LD_PRELOAD=test/libisatty.so ./src/tarantool
+add_library(isatty SHARED isatty.c)
diff --git a/test/isatty.c b/test/isatty.c
new file mode 100644
index 000000000..f716d2903
--- /dev/null
+++ b/test/isatty.c
@@ -0,0 +1,5 @@
+int
+isatty(int fd)
+{
+	return 1;
+}
-- 
2.20.1 (Apple Git-117)

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Tarantool-patches] [PATCH 2/2] console: add line carrying backslash
  2020-06-10  9:43 [Tarantool-patches] [PATCH v2 0/2] console: support of backslash Olga Arkhangelskaia
  2020-06-10  9:43 ` [Tarantool-patches] [PATCH 1/2] test: add libisatty to test local console Olga Arkhangelskaia
@ 2020-06-10  9:43 ` Olga Arkhangelskaia
  2020-06-11 16:48   ` Igor Munkin
  2020-06-16 14:39   ` Alexander Turenko
  2020-06-11 16:49 ` [Tarantool-patches] [PATCH v2 0/2] console: support of backslash Igor Munkin
  2020-07-08 18:42 ` Alexander Turenko
  3 siblings, 2 replies; 9+ messages in thread
From: Olga Arkhangelskaia @ 2020-06-10  9:43 UTC (permalink / raw)
  To: tarantool-patches; +Cc: alexander.turenko

When using interactive console(stdin) instead of \set delimiter <delimiter>
with "\", "\" in the end if line can be used.

Closes #4317
---
 src/box/lua/console.lua       | 18 ++++++++-----
 test/app-tap/gh-4317.test.lua | 51 +++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 6 deletions(-)
 create mode 100755 test/app-tap/gh-4317.test.lua

diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
index 17e2c91b2..4c2c7a132 100644
--- a/src/box/lua/console.lua
+++ b/src/box/lua/console.lua
@@ -573,14 +573,20 @@ local function local_read(self)
             break
         end
         if delim == "" then
-            local lang = box.session.language
-            if not lang or lang == 'lua' then
-                -- stop once a complete Lua statement is entered
-                if local_check_lua(buf) then
+            -- if no delim is set and line ends with the backslash
+            -- continue reading
+            if buf:sub(-1, -1) == '\\' then
+                buf = buf:sub(0, #buf - 1)
+            else
+                local lang = box.session.language
+                if not lang or lang == 'lua' then
+                    -- stop once a complete Lua statement is entered
+                    if local_check_lua(buf) then
+                        break
+                    end
+                else
                     break
                 end
-            else
-                break
             end
         elseif #buf >= #delim and buf:sub(#buf - #delim + 1) == delim then
             buf = buf:sub(0, #buf - #delim)
diff --git a/test/app-tap/gh-4317.test.lua b/test/app-tap/gh-4317.test.lua
new file mode 100755
index 000000000..d7a5063e7
--- /dev/null
+++ b/test/app-tap/gh-4317.test.lua
@@ -0,0 +1,51 @@
+#!/usr/bin/env tarantool
+
+local tap = require('tap')
+local console = require('console')
+local fio = require('fio')
+
+-- The following cases use LD_PRELOAD, so will not work under
+-- Mac OS X.
+if jit.os == 'OSX' then
+    os.exit(0)
+end
+
+test = tap.test("console")
+test:plan(1)
+
+-- Allow to run the test from the repository root without
+-- test-run:
+--
+-- $ ./src/tarantool test/app-tap/gh-4317.test.lua
+--
+-- It works at least for in-source build.
+local is_under_test_run = pcall(require, 'test_run')
+local isatty_lib_dir = is_under_test_run and '../..' or 'test'
+local isatty_lib_path = fio.pathjoin(isatty_lib_dir, 'libisatty.so')
+local saved_path = os.getenv('PATH')
+if not is_under_test_run then
+    os.setenv('PATH', './src:', saved_path)
+end
+
+local tarantool_command = "local a = 0 \\\nfor i = 1, 10 do\na = a + i\nend \\\nprint(a)"
+local result_str =  [[tarantool> local a = 0 \
+         > for i = 1, 10 do
+         > a = a + i
+         > end \
+         > print(a)
+55
+---
+...
+
+tarantool> ]]
+local cmd = ([[printf '%s\n' | LD_PRELOAD='%s' tarantool ]] ..
+[[2>/dev/null]]):format(tarantool_command, isatty_lib_path)
+local fh = io.popen(cmd, 'r')
+-- Readline on CentOS 7 produces \e[?1034h escape
+-- sequence before tarantool> prompt, remove it.
+local result = fh:read('*a'):gsub('\x1b%[%?1034h', '')
+fh:close()
+test:is(result, result_str, "Backslash")
+
+test:check()
+os.exit(0)
-- 
2.20.1 (Apple Git-117)

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Tarantool-patches] [PATCH 1/2] test: add libisatty to test local console
  2020-06-10  9:43 ` [Tarantool-patches] [PATCH 1/2] test: add libisatty to test local console Olga Arkhangelskaia
@ 2020-06-11 16:47   ` Igor Munkin
  0 siblings, 0 replies; 9+ messages in thread
From: Igor Munkin @ 2020-06-11 16:47 UTC (permalink / raw)
  To: Olga Arkhangelskaia; +Cc: tarantool-patches, alexander.turenko

Olya,

Thanks for the patch! I see no reason to introduce this auxiliary
library as a separate patch. Please consider some nits below.

On 10.06.20, Olga Arkhangelskaia wrote:
> From: Olga <arkholga@tarantool.org>
> 
> Local console is impossible to test within pipe, tatantool -i.

Typo: s/within/with/.
Typo: s/tatantool/tarantool/.

Minor: it's worth to describe the exact behaviour preventing tarantool
start with interactive console (i.e. isatty check) and mention the
issue[1] confirming that such behaviour is not OK.

> isatty lib was added to overcome isatty check and test local console.

Typo: s/overcome/overload/.

> Hack by Alexandr Turenko.

Minor: I failed to find such practice in our repo. If you want to
mention Sasha as author of this way to test local console, I guess you
can just add a Co-authored-by tag (if Sasha doesn't mind).

I also think, it's worth to mention the issue that motivates this patch.
Something like "Needed for #4317" is enough.

> ---
>  test/CMakeLists.txt | 8 ++++++++
>  test/isatty.c       | 5 +++++

Minor: it's better to place this library to test/app-tap directory. In
this case it simply doesn't spoil the root one.

>  2 files changed, 13 insertions(+)
>  create mode 100644 test/isatty.c
> 

<snipped>

> -- 
> 2.20.1 (Apple Git-117)
> 

[1]: https://github.com/tarantool/tarantool/issues/5064

-- 
Best regards,
IM

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Tarantool-patches] [PATCH 2/2] console: add line carrying backslash
  2020-06-10  9:43 ` [Tarantool-patches] [PATCH 2/2] console: add line carrying backslash Olga Arkhangelskaia
@ 2020-06-11 16:48   ` Igor Munkin
  2020-06-16 14:39   ` Alexander Turenko
  1 sibling, 0 replies; 9+ messages in thread
From: Igor Munkin @ 2020-06-11 16:48 UTC (permalink / raw)
  To: Olga Arkhangelskaia; +Cc: tarantool-patches, alexander.turenko

Olya,

Thanks for the patch!

On 10.06.20, Olga Arkhangelskaia wrote:
> When using interactive console(stdin) instead of \set delimiter <delimiter>
> with "\", "\" in the end if line can be used.

I propose to reword commit message a bit to make it clearer. Consider
the following:
| Before this patch one has to explicitly set delimiter different from
| newline ('\n') symbol when using interactive console.
|
| The changes make possible to split console input into multiple lines
| via backslash ('\\') at the end of the line instead of setting a
| delimiter.
Feel free to change it on your own.

> 
> Closes #4317
> ---
>  src/box/lua/console.lua       | 18 ++++++++-----
>  test/app-tap/gh-4317.test.lua | 51 +++++++++++++++++++++++++++++++++++
>  2 files changed, 63 insertions(+), 6 deletions(-)
>  create mode 100755 test/app-tap/gh-4317.test.lua
> 
> diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
> index 17e2c91b2..4c2c7a132 100644
> --- a/src/box/lua/console.lua
> +++ b/src/box/lua/console.lua
> @@ -573,14 +573,20 @@ local function local_read(self)
>              break
>          end
>          if delim == "" then
> -            local lang = box.session.language
> -            if not lang or lang == 'lua' then
> -                -- stop once a complete Lua statement is entered
> -                if local_check_lua(buf) then
> +            -- if no delim is set and line ends with the backslash
> +            -- continue reading
> +            if buf:sub(-1, -1) == '\\' then
> +                buf = buf:sub(0, #buf - 1)
> +            else
> +                local lang = box.session.language
> +                if not lang or lang == 'lua' then
> +                    -- stop once a complete Lua statement is entered
> +                    if local_check_lua(buf) then
> +                        break
> +                    end
> +                else
>                      break
>                  end
> -            else
> -                break
>              end
>          elseif #buf >= #delim and buf:sub(#buf - #delim + 1) == delim then
>              buf = buf:sub(0, #buf - #delim)
> diff --git a/test/app-tap/gh-4317.test.lua b/test/app-tap/gh-4317.test.lua
> new file mode 100755
> index 000000000..d7a5063e7
> --- /dev/null
> +++ b/test/app-tap/gh-4317.test.lua

Minor: it would be nice to describe the bug in test name using 2-3
words[1]:
| gh-####-description.test.lua

> @@ -0,0 +1,51 @@
> +#!/usr/bin/env tarantool
> +
> +local tap = require('tap')
> +local console = require('console')

The line above is excess.

> +local fio = require('fio')
> +
> +-- The following cases use LD_PRELOAD, so will not work under
> +-- Mac OS X.
> +if jit.os == 'OSX' then
> +    os.exit(0)
> +end
> +
> +test = tap.test("console")
> +test:plan(1)
> +
> +-- Allow to run the test from the repository root without
> +-- test-run:
> +--
> +-- $ ./src/tarantool test/app-tap/gh-4317.test.lua
> +--
> +-- It works at least for in-source build.
> +local is_under_test_run = pcall(require, 'test_run')
> +local isatty_lib_dir = is_under_test_run and '../..' or 'test'
> +local isatty_lib_path = fio.pathjoin(isatty_lib_dir, 'libisatty.so')
> +local saved_path = os.getenv('PATH')
> +if not is_under_test_run then
> +    os.setenv('PATH', './src:', saved_path)

IMHO, using relative paths for such case is too fragile:
| $ ./tarantool ../test/app-tap/gh-4317.test.lua
| TAP version 13
| 1..1
| not ok - Backslash
|   ---
|   filename: ../test/app-tap/gh-4317.test.lua
|   trace:
|   - line: 0
|     source: '@../test/app-tap/gh-4317.test.lua'
|     filename: ../test/app-tap/gh-4317.test.lua
|     what: main
|     namewhat: 
|     src: ../test/app-tap/gh-4317.test.lua
|   line: 0
|   expected: "tarantool> local a = 0 \\\n         > for i = 1, 10 do\n         > a =
|     a + i\n         > end \\\n         > print(a)\n55\n---\n...\n\ntarantool> "
|   got: 
|   ...
| # failed subtest: 1

Yes, it's not the way you described in the comment above, *but* one can
try to run the chunk this way. You can use arg[-1] that provides the Lua
interpreter running this script. Consider the following:
| $ cat test.lua
| print(inspect(arg))
| $ LUA_PATH=~/.luarocks/share/lua/5.1/?.lua ./src/tarantool -linspect test.lua
| {
|   [-1] = "/home/imun/tarantool/src/tarantool",
|   [0] = "test.lua"
| }

I attached the diff at the end of reply: it makes possible to run the
given test any desired way for both in-source and out-of-source builds
(NB: test Lua files for out-of-source build are located within the
source directory).

> +end
> +
> +local tarantool_command = "local a = 0 \\\nfor i = 1, 10 do\na = a + i\nend \\\nprint(a)"
> +local result_str =  [[tarantool> local a = 0 \
> +         > for i = 1, 10 do
> +         > a = a + i
> +         > end \
> +         > print(a)
> +55
> +---
> +...
> +
> +tarantool> ]]
> +local cmd = ([[printf '%s\n' | LD_PRELOAD='%s' tarantool ]] ..
> +[[2>/dev/null]]):format(tarantool_command, isatty_lib_path)
> +local fh = io.popen(cmd, 'r')
> +-- Readline on CentOS 7 produces \e[?1034h escape
> +-- sequence before tarantool> prompt, remove it.
> +local result = fh:read('*a'):gsub('\x1b%[%?1034h', '')
> +fh:close()
> +test:is(result, result_str, "Backslash")
> +
> +test:check()
> +os.exit(0)

Please use the following idiom[1] in your tests:
| Use os.exit(test:check() and 0 or 1) at the end.

> -- 
> 2.20.1 (Apple Git-117)
> 

Here is the diff:

================================================================================

diff --git a/test/app-tap/gh-4317.test.lua b/test/app-tap/gh-4317.test.lua
index d7a5063e7..82c48d302 100755
--- a/test/app-tap/gh-4317.test.lua
+++ b/test/app-tap/gh-4317.test.lua
@@ -2,7 +2,6 @@
 
 local tap = require('tap')
 local console = require('console')
-local fio = require('fio')
 
 -- The following cases use LD_PRELOAD, so will not work under
 -- Mac OS X.
@@ -13,19 +12,10 @@ end
 test = tap.test("console")
 test:plan(1)
 
--- Allow to run the test from the repository root without
--- test-run:
---
--- $ ./src/tarantool test/app-tap/gh-4317.test.lua
---
--- It works at least for in-source build.
-local is_under_test_run = pcall(require, 'test_run')
-local isatty_lib_dir = is_under_test_run and '../..' or 'test'
-local isatty_lib_path = fio.pathjoin(isatty_lib_dir, 'libisatty.so')
-local saved_path = os.getenv('PATH')
-if not is_under_test_run then
-    os.setenv('PATH', './src:', saved_path)
-end
+-- Locate libisattyLso related to the running tarantool binary.
+local binary = arg[-1]
+local isatty_lib_path = binary:gsub('tarantool$', '../test/libisatty.so')
+assert(isatty_lib_path:match('libisatty.so$'), 'Failed to locate libisatty.so')
 
 local tarantool_command = "local a = 0 \\\nfor i = 1, 10 do\na = a + i\nend \\\nprint(a)"
 local result_str =  [[tarantool> local a = 0 \
@@ -38,8 +28,8 @@ local result_str =  [[tarantool> local a = 0 \
 ...
 
 tarantool> ]]
-local cmd = ([[printf '%s\n' | LD_PRELOAD='%s' tarantool ]] ..
-[[2>/dev/null]]):format(tarantool_command, isatty_lib_path)
+local cmd = ([[printf '%s\n' | LD_PRELOAD='%s' %s 2>/dev/null]])
+    :format(tarantool_command, isatty_lib_path, binary)
 local fh = io.popen(cmd, 'r')
 -- Readline on CentOS 7 produces \e[?1034h escape
 -- sequence before tarantool> prompt, remove it.

================================================================================

[1]: https://github.com/tarantool/doc/issues/1004

-- 
Best regards,
IM

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 0/2] console: support of backslash
  2020-06-10  9:43 [Tarantool-patches] [PATCH v2 0/2] console: support of backslash Olga Arkhangelskaia
  2020-06-10  9:43 ` [Tarantool-patches] [PATCH 1/2] test: add libisatty to test local console Olga Arkhangelskaia
  2020-06-10  9:43 ` [Tarantool-patches] [PATCH 2/2] console: add line carrying backslash Olga Arkhangelskaia
@ 2020-06-11 16:49 ` Igor Munkin
  2020-07-08 18:42 ` Alexander Turenko
  3 siblings, 0 replies; 9+ messages in thread
From: Igor Munkin @ 2020-06-11 16:49 UTC (permalink / raw)
  To: Olga Arkhangelskaia; +Cc: tarantool-patches, alexander.turenko

Olya,

Thanks for the series! I left all patch-related comments in the
corresponding replies. Here are also a couple not related to the code
but the patchset itself.

On 10.06.20, Olga Arkhangelskaia wrote:
> @ChangeLog
> While using local console one can use '\' to carry the line without
> setting the delimiter.
> Works only with local console.

I guess the last line can be dropped, since you've already mentioned
local console in the first line. Also please add the issue at the end of
the sentence.

> 
> Changes v2:
> Added test case

Minor: Please look how the changes between versions should be listed in
our contribution guide[1] (or in your previous patches).

> 
> Olga (1):

This looks like a mess with the author both here and on the remote
branch. Please fix it before the patch is applied to the stable ones.

>   test: add libisatty to test local console
> 
> Olga Arkhangelskaia (1):
>   console: add line carrying backslash
> 
>  src/box/lua/console.lua       | 18 ++++++++-----
>  test/CMakeLists.txt           |  8 ++++++
>  test/app-tap/gh-4317.test.lua | 51 +++++++++++++++++++++++++++++++++++
>  test/isatty.c                 |  5 ++++
>  4 files changed, 76 insertions(+), 6 deletions(-)
>  create mode 100755 test/app-tap/gh-4317.test.lua
>  create mode 100644 test/isatty.c
> 
> -- 
> 2.20.1 (Apple Git-117)
> 

[1]: https://www.tarantool.io/en/doc/2.2/dev_guide/developer_guidelines/

-- 
Best regards,
IM

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Tarantool-patches] [PATCH 2/2] console: add line carrying backslash
  2020-06-10  9:43 ` [Tarantool-patches] [PATCH 2/2] console: add line carrying backslash Olga Arkhangelskaia
  2020-06-11 16:48   ` Igor Munkin
@ 2020-06-16 14:39   ` Alexander Turenko
  1 sibling, 0 replies; 9+ messages in thread
From: Alexander Turenko @ 2020-06-16 14:39 UTC (permalink / raw)
  To: Olga Arkhangelskaia; +Cc: tarantool-patches

On Wed, Jun 10, 2020 at 12:43:54PM +0300, Olga Arkhangelskaia wrote:
> When using interactive console(stdin) instead of \set delimiter <delimiter>
> with "\", "\" in the end if line can be used.

local_read() also works on a console client side, so, say, `tarantoolctl
connect` will work as expected in interactive mode.

However I think it is good to left more freedom for scripting with, say, netcat
and support the feature at console server side too. It looks quite simple:

@@ -640,9 +644,12 @@ local function local_print(self, output)
 end
 
 --
--- Read command from connected client console.listen()
+-- Read a line or a chunk if delimiter is not empty.
 --
-local function client_read(self)
+-- The value returned without delimiter (if any) and trailing
+-- newline character.
+--
+local function client_read_line(self)
     --
     -- Byte sequences that come over the network and come from
     -- the local client console may have a different terminal
@@ -663,6 +670,34 @@ local function client_read(self)
     return buf:sub(1, -#self.delimiter-2)
 end
 
+--
+-- Read command from connected client console.listen()
+--
+local function client_read(self)
+    local buf = ''
+    while true do
+        local line = client_read_line(self)
+        if line == nil then
+            -- EOF or error.
+            --
+            -- Note: Even if `buf` is not empty, skip unfinished
+            -- command.
+            return nil
+        end
+        buf = buf .. line
+        if line == '' then
+            -- Continue if the line is empty.
+        elseif self.delimiter == '' and buf:endswith('\\') then
+            -- Continue reading if the line ends with the
+            -- backslash and no delimiter is set.
+            buf = buf:sub(1, -2)
+        else
+            break
+        end
+    end
+    return buf
+end
+
 --
 -- Print result to connected client from console.listen()
 --

> 
> Closes #4317
> ---
>  src/box/lua/console.lua       | 18 ++++++++-----
>  test/app-tap/gh-4317.test.lua | 51 +++++++++++++++++++++++++++++++++++
>  2 files changed, 63 insertions(+), 6 deletions(-)
>  create mode 100755 test/app-tap/gh-4317.test.lua
> 
> diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
> index 17e2c91b2..4c2c7a132 100644
> --- a/src/box/lua/console.lua
> +++ b/src/box/lua/console.lua
> @@ -573,14 +573,20 @@ local function local_read(self)
>              break
>          end
>          if delim == "" then
> -            local lang = box.session.language
> -            if not lang or lang == 'lua' then
> -                -- stop once a complete Lua statement is entered
> -                if local_check_lua(buf) then
> +            -- if no delim is set and line ends with the backslash
> +            -- continue reading
> +            if buf:sub(-1, -1) == '\\' then
> +                buf = buf:sub(0, #buf - 1)

I suggest to use -2 instead of #buf -1 to eliminate extra string length
operation. Also, all indices in Lua starts from 1.

> +            else
> +                local lang = box.session.language
> +                if not lang or lang == 'lua' then
> +                    -- stop once a complete Lua statement is entered
> +                    if local_check_lua(buf) then
> +                        break
> +                    end
> +                else
>                      break
>                  end
> -            else
> -                break
>              end
>          elseif #buf >= #delim and buf:sub(#buf - #delim + 1) == delim then
>              buf = buf:sub(0, #buf - #delim)

We can decrease nesting level this way:

diff --git a/src/box/lua/console.lua b/src/box/lua/console.lua
index 6ea27a393..effb0f0b5 100644
--- a/src/box/lua/console.lua
+++ b/src/box/lua/console.lua
@@ -604,7 +604,11 @@ local function local_read(self)
         if buf:sub(1, 1) == '\\' then
             break
         end
-        if delim == "" then
+        if delim == '' and line:endswith('\\') then
+            -- Continue reading if the line ends with the
+            -- backslash and no delimiter is set.
+            buf = buf:sub(1, -2)
+        elseif delim == "" then
             local lang = box.session.language
             if not lang or lang == 'lua' then
                 -- stop once a complete Lua statement is entered

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 0/2] console: support of backslash
  2020-06-10  9:43 [Tarantool-patches] [PATCH v2 0/2] console: support of backslash Olga Arkhangelskaia
                   ` (2 preceding siblings ...)
  2020-06-11 16:49 ` [Tarantool-patches] [PATCH v2 0/2] console: support of backslash Igor Munkin
@ 2020-07-08 18:42 ` Alexander Turenko
  2020-07-09  6:01   ` Olga Arkhangelskaia
  3 siblings, 1 reply; 9+ messages in thread
From: Alexander Turenko @ 2020-07-08 18:42 UTC (permalink / raw)
  To: Olga Arkhangelskaia; +Cc: tarantool-patches

First, I don't see any updates in the mailing list since v2, where Igor
and me responds. However the branch is updated and I'll left a couple of
comment regarding the patch on the branch.

Typo: tatantool.

Please, use spell checker. I don't remember a patch without typos from
you.

Wrong indent in console.lua.

No empty line between client_read() and client_print().

gh-4317.test.lua -- please, follow our naming guidelines.

CI fails on Mac OS X (see [1]):

 | [017] app-tap/gh-4317.test.lua
 | [017] TAP13 parse failed (Missing plan in the TAP source).

Move all requires to the top of the lua file (in test).

Run luacheck on your test file. Several warnings are relevant to our
guidelines: say, about using local (see [2]) or usnused variables.
However we agreed that some warnings are should be considered
false-positive for us: say, variables hiding and redefinitions.

Remove print().

Remove a space before a colon.

----

I don't ever started to look into the logic of changes and already has a
half screen of comments. It is not acceptable and just wastes my time.

If you'll not do thorough self-review (at least points mentioned in to
[3]), I'll not do review for you. Don't take it personal, it is how we
work in the team.

I can do review for the test, but console.lua changes are partially
written by me, so I hope Igor will do review for this part (when you'll
send the next version).

So, the plan is the following:

- Rebase to master, fix test of Mac OS, **do self-review carefully**.
  Ask questions what is considered good style and bad if it is unclear
  (ask in the team chat or from me directly).
- Send v3 to Igor and me.

The ball is in your court.

WBR, Alexander Turenko.

[1]: https://gitlab.com/tarantool/tarantool/-/jobs/629030801
[2]: https://github.com/tarantool/doc/issues/1004
[3]: https://github.com/tarantool/tarantool/wiki/Code-review-procedure

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 0/2] console: support of backslash
  2020-07-08 18:42 ` Alexander Turenko
@ 2020-07-09  6:01   ` Olga Arkhangelskaia
  0 siblings, 0 replies; 9+ messages in thread
From: Olga Arkhangelskaia @ 2020-07-09  6:01 UTC (permalink / raw)
  To: Alexander Turenko; +Cc: tarantool-patches

Hi Alexandr!

You have reviewed raw branch and wasted your time because of you and not 
because of me.

08.07.2020 21:42, Alexander Turenko пишет:
> First, I don't see any updates in the mailing list since v2, where Igor
> and me responds. However the branch is updated and I'll left a couple of
> comment regarding the patch on the branch.
>
> Typo: tatantool.
>
> Please, use spell checker. I don't remember a patch without typos from
> you.
>
> Wrong indent in console.lua.
>
> No empty line between client_read() and client_print().
>
> gh-4317.test.lua -- please, follow our naming guidelines.
>
> CI fails on Mac OS X (see [1]):
>
>   | [017] app-tap/gh-4317.test.lua
>   | [017] TAP13 parse failed (Missing plan in the TAP source).
>
> Move all requires to the top of the lua file (in test).
>
> Run luacheck on your test file. Several warnings are relevant to our
> guidelines: say, about using local (see [2]) or usnused variables.
> However we agreed that some warnings are should be considered
> false-positive for us: say, variables hiding and redefinitions.
>
> Remove print().
>
> Remove a space before a colon.
>
> ----
>
> I don't ever started to look into the logic of changes and already has a
> half screen of comments. It is not acceptable and just wastes my time.
>
> If you'll not do thorough self-review (at least points mentioned in to
> [3]), I'll not do review for you. Don't take it personal, it is how we
> work in the team.
>
> I can do review for the test, but console.lua changes are partially
> written by me, so I hope Igor will do review for this part (when you'll
> send the next version).
>
> So, the plan is the following:
>
> - Rebase to master, fix test of Mac OS, **do self-review carefully**.
>    Ask questions what is considered good style and bad if it is unclear
>    (ask in the team chat or from me directly).
> - Send v3 to Igor and me.
>
> The ball is in your court.
>
> WBR, Alexander Turenko.
>
> [1]: https://gitlab.com/tarantool/tarantool/-/jobs/629030801
> [2]: https://github.com/tarantool/doc/issues/1004
> [3]: https://github.com/tarantool/tarantool/wiki/Code-review-procedure

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-07-09  6:01 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-10  9:43 [Tarantool-patches] [PATCH v2 0/2] console: support of backslash Olga Arkhangelskaia
2020-06-10  9:43 ` [Tarantool-patches] [PATCH 1/2] test: add libisatty to test local console Olga Arkhangelskaia
2020-06-11 16:47   ` Igor Munkin
2020-06-10  9:43 ` [Tarantool-patches] [PATCH 2/2] console: add line carrying backslash Olga Arkhangelskaia
2020-06-11 16:48   ` Igor Munkin
2020-06-16 14:39   ` Alexander Turenko
2020-06-11 16:49 ` [Tarantool-patches] [PATCH v2 0/2] console: support of backslash Igor Munkin
2020-07-08 18:42 ` Alexander Turenko
2020-07-09  6:01   ` Olga Arkhangelskaia

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox