* [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
* 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
* [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 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 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 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 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