From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp60.i.mail.ru (smtp60.i.mail.ru [217.69.128.40]) (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 6B64A45C304 for ; Tue, 15 Dec 2020 13:06:35 +0300 (MSK) References: <6265c0f8-3b96-04db-0bcd-6bb77dc9fe71@tarantool.org> <31bded2b-32e0-5b65-92d1-9ef08415d3fc@tarantool.org> From: Leonid Vasiliev Message-ID: Date: Tue, 15 Dec 2020 13:05:34 +0300 MIME-Version: 1.0 In-Reply-To: <31bded2b-32e0-5b65-92d1-9ef08415d3fc@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Tarantool-patches] [PATCH 1/4] test: convert print to function and make quotes use consistent List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Bronnikov , Vladislav Shpilevoy , tarantool-patches@dev.tarantool.org Cc: alexander.turenko@tarantool.org Hi! I looked at the commit 1f34abf6e059532ddfa097e4c1a86030d34341a9 on the branch, because not all the diff has been attached here (or I can't find them). Not all multi-line prints were reverted. Let's finish the revert. As for the rest of the changes:"I think this patch can be considered complete." (LGTM) On 14.12.2020 14:44, Sergey Bronnikov via Tarantool-patches wrote: > Hello, > > thanks for review! > > On 13.12.2020 20:58, Vladislav Shpilevoy wrote: >> Hi! Thanks for the patch! >> >> Finally, http_client.test.lua works on my machine. >> >> I don't see the cover letter, with issue and branch links. >> I assume the branch is ligurio/gh-5538-support-python3. > > Yep, it's a right branch. Cover letter is here [1], first time it was > send without ML address. > >> See 12 minor comments below. >> >> On 11.12.2020 09:42, Sergey Bronnikov via Tarantool-patches wrote: >>> From: Sergey Bronnikov >>> >>> - convert print statement to function. In a Python 3 'print' becomes a >>> function, see [1]. Patch makes 'print' in a regression tests >>> compatible with >>> Python 3. >>> - according to PEP8, mixing using double quotes and quotes in a >>> project looks >>> inconsistent. Patch makes using quotes with strings consistent.> - >>> print multiline strings with multiple print() >> 1. Why? Is it according to PEP too? The same for '%'. >> What is wrong with using multiline strings? > > On first iteration I have converted print statements to functions as it > is required by Python 3. > > Leonid asked me to fix code style of strings too and I did it on second > iteration, see conversation here [2]. > > Nothing wrong with multi-line prints, but for my taste it looks ugly. > > I don't want to have yet another conversation about code style, if you > are happy with multi-line strings in print - it's fine. > > So I reverted changes related to multi-line strings because the main > purpose of the series is Python 3 support. > >> >>> - use "format()" instead of "%" everywhere >>> 1. https://docs.python.org/3/whatsnew/3.0.html#print-is-a-function >>> >>> Part of #5538 >>> --- >>> diff --git a/test/box-py/args.test.py b/test/box-py/args.test.py >>> index c0fac9038..f1b840a85 100644 >>> --- a/test/box-py/args.test.py >>> +++ b/test/box-py/args.test.py >>> @@ -1,3 +1,5 @@ >>> +from __future__ import print_function >> 2. In this file I don't see you changing 'print' to 'print()' >> anywhere. So why is this line needed? In this file I only see >> "print" as a part of strings. As a word. Not as a function. > You are right. Removed import and force-pushed. >> >>> + >>>   import sys >>>   import os >>>   import re >>> diff --git a/test/box-py/bad_trigger.result >>> b/test/box-py/bad_trigger.result >>> index 5d064b764..42cd9a7e8 100644 >>> --- a/test/box-py/bad_trigger.result >>> +++ b/test/box-py/bad_trigger.result >>> @@ -1,8 +1,6 @@ >>> - >>>    # >>>    # if on_connect() trigger raises an exception, the connection is >>> dropped >>>    # >>> - >> 3. Why is this file changed? It is .result file. Your changes are just >> refactoring, so no .result files should be changed. > Added newlines. >> >>>   nosuchfunction = nil >>>   --- >>>   ... >>> diff --git a/test/box-py/bad_trigger.test.py >>> b/test/box-py/bad_trigger.test.py >>> index 7d200b921..789fe8045 100644 >>> --- a/test/box-py/bad_trigger.test.py >>> +++ b/test/box-py/bad_trigger.test.py >>> @@ -24,12 +24,12 @@ conn.connect() >>>   s = conn.socket >>>   # Read greeting >>> -print 'greeting: ', len(s.recv(IPROTO_GREETING_SIZE)) == >>> IPROTO_GREETING_SIZE >>> +print("greeting:  {}".format(len(s.recv(IPROTO_GREETING_SIZE)) == >>> IPROTO_GREETING_SIZE)) >>>   # Read error packet >>>   IPROTO_FIXHEADER_SIZE = 5 >>>   fixheader = s.recv(IPROTO_FIXHEADER_SIZE) >>> -print 'fixheader: ', len(fixheader) == IPROTO_FIXHEADER_SIZE >>> +print("fixheader:  {}".format(len(fixheader) == IPROTO_FIXHEADER_SIZE)) >> 4. There was a single whitespace. You made it double. Why? The >> same for the change above. And in one place below. > > Python2 print automatically adds a whitespace before an argument: > > Python 2.7.18 (default, Aug  4 2020, 11:16:42) > [GCC 9.3.0] on linux2 > Type "help", "copyright", "credits" or "license" for more information. > >>> print "X", 3 > X 3 > >>> from __future__ import print_function > >>> print("X{}".format(3)) > X3 > >>> > > To produce same output as in a .result file additional whitespace has > been added in a code. > >> >>>   unpacker.feed(fixheader) >>>   packet_len = unpacker.unpack() >>>   packet = s.recv(packet_len) >>> @@ -38,9 +38,9 @@ unpacker.feed(packet) >>>   # Parse packet >>>   header = unpacker.unpack() >>>   body = unpacker.unpack() >>> -print 'error code', (header[IPROTO_CODE] & (REQUEST_TYPE_ERROR - 1)) >>> -print 'error message: ', body[IPROTO_ERROR] >>> -print 'eof:', len(s.recv(1024)) == 0 >>> +print("error code {}".format((header[IPROTO_CODE] & >>> (REQUEST_TYPE_ERROR - 1)))) >>> +print("error message:  {}".format(body[IPROTO_ERROR])) >>> +print("eof: {}".format(len(s.recv(1024)) == 0)) >>>   s.close() >>>   server.admin("box.session.on_connect(nil, f1)") >>> diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result >>> index 0876e77a6..865302698 100644 >>> --- a/test/box-py/bootstrap.result >>> +++ b/test/box-py/bootstrap.result >>> @@ -165,7 +165,7 @@ box.space._user:select{} >>>     - [3, 1, 'replication', 'role', {}] >>>     - [31, 1, 'super', 'role', {}] >>>   ... >>> -for _, v in box.space._func:pairs{} do r = {} table.insert(r, >>> v:update({{"=", 18, ""}, {"=", 19, ""}})) return r end >>> +for _, v in box.space._func:pairs{} do r = {} table.insert(r, >>> v:update({{'=', 18, ''}, {'=', 19, ''}})) return r end >> 5. This is Lua. Not python. Why did you change it? > > I just made using quotes consistent in a Python code. Outer quotes are > double quote and internal quote is a single one. > > --- a/test/box-py/bootstrap.test.py > +++ b/test/box-py/bootstrap.test.py > @@ -1,11 +1,11 @@ > -server.admin('box.internal.bootstrap()') > -server.admin('box.space._schema:select{}') > -server.admin('box.space._cluster:select{}') > -server.admin('box.space._space:select{}') > -server.admin('box.space._index:select{}') > -server.admin('box.space._user:select{}') > -server.admin('for _, v in box.space._func:pairs{} do r = {} > table.insert(r, v:update({{"=", 18, ""}, {"=", 19, ""}})) return r end') > -server.admin('box.space._priv:select{}') > +server.admin("box.internal.bootstrap()") > +server.admin("box.space._schema:select{}") > +server.admin("box.space._cluster:select{}") > +server.admin("box.space._space:select{}") > +server.admin("box.space._index:select{}") > +server.admin("box.space._user:select{}") > +server.admin("for _, v in box.space._func:pairs{} do r = {} > table.insert(r, v:update({{'=', 18, ''}, {'=', 19, ''}})) return r end") > +server.admin("box.space._priv:select{}") > >  # Cleanup >  server.stop() > >> >>>   --- >>>   - - [1, 1, 'box.schema.user.info', 1, 'LUA', '', 'function', [], >>> 'any', 'none', 'none', >>>       false, false, true, ['LUA'], {}, '', '', ''] >>> diff --git a/test/box-py/call.result b/test/box-py/call.result >>> index d340ed6fa..e6b7b8dc9 100644 >>> --- a/test/box-py/call.result >>> +++ b/test/box-py/call.result >>> @@ -536,10 +536,10 @@ function f(...) return ... end >>>   call f({'k2': 'v2', 'k1': 'v1'}) >>>   --- >>>   - {'k2': 'v2', 'k1': 'v1'} >>> -eval (return space:auto_increment({"transaction"}))() >>> +eval (return space:auto_increment({'transaction'}))() >>>   --- >>>   - [1, 'transaction'] >>> -function f(...) return space:auto_increment({"transaction"}) end >>> +function f(...) return space:auto_increment({'transaction'}) end >> 6. This whole file is Lua output. Why is it changed? > > Same as in answer to the previous comment. Using single quote inside > double quotes. > > Relevant part of code: > >  # Transactions > -test('space:auto_increment({"transaction"})') > -test('space:select{}') > -test('box.begin(), space:auto_increment({"failed"}), box.rollback()') > -test('space:select{}') > -test('require("fiber").sleep(0)') > +test("space:auto_increment({'transaction'})") > +test("space:select{}") > +test("box.begin(), space:auto_increment({'failed'}), box.rollback()") > +test("space:select{}") > +test("require('fiber').sleep(0)") > >>>   --- >>>   ... >>>   call f() >>> diff --git a/test/box-py/iproto.test.py b/test/box-py/iproto.test.py >>> index cdd1a71c5..5eccd40d3 100644 >>> --- a/test/box-py/iproto.test.py >>> +++ b/test/box-py/iproto.test.py >>> @@ -11,46 +13,42 @@ from lib.tarantool_connection import >>> TarantoolConnection >>>   admin("box.schema.user.grant('guest', 'read,write,execute', >>> 'universe')") >>> -print """ >>> -# >>> -# iproto packages test >>> -# >>> -""" >>> +print("#") >>> +print("# iproto packages test") >>> +print("#") >>> -# opeing new connection to tarantool/box >>> +# opening new connection to tarantool/box >>>   conn = TarantoolConnection(server.iproto.host, server.iproto.port) >>>   conn.connect() >>>   s = conn.socket >>> -print """ >>> -# Test bug #899343 (server assertion failure on incorrect packet) >>> -""" >>> -print "# send the package with invalid length" >>> -invalid_request = struct.pack('>> -print s.send(invalid_request) >>> -print "# check that is server alive" >>> -print iproto.py_con.ping() > 0 >>> +print("# Test bug #899343 (server assertion failure on incorrect >>> packet)") >>> +print("# send the package with invalid length") >>> +invalid_request = struct.pack(">> +print(s.send(invalid_request)) >>> +print("# check that is server alive") >>> +print(iproto.py_con.ping() > 0) >>>   # closing connection >>>   s.close() >>>   key_names = {} >>> -for (k,v) in globals().items(): >>> -    if type(k) == str and k.startswith('IPROTO_') and type(v) == int: >>> +for (k,v) in list(globals().items()): >> 7. Why did you add list() call? I don't see in the commit >> message any mentioning of why is it bad to use items() without >> list(). The same below. > > To fix exceptions when Python 3 used: > > [006]   File "box-py/iproto.test.py", line 40, in > [006]     for (k,v) in globals().items(): > [006] RuntimeError: dictionary changed size during iteration > >> >> I suspect this has something to do with the second commit, >> and therefore belongs to it. > Right, moved to a separate commit. >> >>> +    if type(k) == str and k.startswith("IPROTO_") and type(v) == int: >>>           key_names[v] = k >>> @@ -59,36 +57,34 @@ def test(header, body): >>>       try: >>>           s.send(query) >>>       except OSError as e: >>> -        print '   => ', 'Failed to send request' >>> +        print("   => ", "Failed to send request") >>>       c.close() >>> -    print iproto.py_con.ping() > 0 >>> +    print(iproto.py_con.ping() > 0) >>> -print """ >>> -#  Test gh-206 "Segfault if sending IPROTO package without `KEY` field" >>> -""" >>> +print("#  Test gh-206 'Segfault if sending IPROTO package without >>> `KEY` field'") >> 8. You changed the output. Previously it had one empty line before and >> after the text. The same in some other places in this commit. No .result >> files should be changed. >> >>> diff --git a/test/replication-py/cluster.test.py >>> b/test/replication-py/cluster.test.py >>> index 088ca9c34..c770a9bc9 100644 >>> --- a/test/replication-py/cluster.test.py >>> +++ b/test/replication-py/cluster.test.py >>> @@ -27,56 +29,59 @@ replica_uuid = str(uuid.uuid4()) >>>   ## Universal read permission is required to perform JOIN/SUBSCRIBE >>>   rows = list(server.iproto.py_con.join(replica_uuid)) >>> -print len(rows) == 1 and rows[0].return_message.find('Read access') >>> >= 0 and \ >>> -    'ok' or 'not ok', '-', 'join without read permissions on universe' >>> +status = len(rows) == 1     clge.find("Read access") >= 0 and \ >>> +        "ok" or "not ok" >> 9. I see, that at least in this file the indentation step is 4 >> symbols. Why did you change it to 8? > > reverted indentation here and in two lines below: > > --- a/test/replication-py/cluster.test.py > +++ b/test/replication-py/cluster.test.py > @@ -30,18 +30,18 @@ replica_uuid = str(uuid.uuid4()) >  ## Universal read permission is required to perform JOIN/SUBSCRIBE >  rows = list(server.iproto.py_con.join(replica_uuid)) >  status = len(rows) == 1 and rows[0].return_message.find("Read access") > >= 0 and \ > -        "ok" or "not ok" > +    "ok" or "not ok" >  print("{} - join without read permissions on universe".format(status)) >  rows = list(server.iproto.py_con.subscribe(cluster_uuid, replica_uuid)) >  status = len(rows) == 1 and rows[0].return_message.find("Read access") > >= 0 and \ > -        "ok" or "not ok" > +    "ok" or "not ok" >  print("{} - subscribe without read permissions on > universe".format(status)) >  ## Write permission to space `_cluster` is required to perform JOIN >  server.admin("box.schema.user.grant('guest', 'read', 'universe')") >  server.iproto.reconnect() # re-connect with new permissions >  rows = list(server.iproto.py_con.join(replica_uuid)) >  status = len(rows) == 1 and rows[0].return_message.find("Write > access") >= 0 and \ > -        "ok" or "not ok" > +    "ok" or "not ok" >  print("{} - join without write permissions to _cluster".format(status)) > >  def check_join(msg): > >> >>> +print("{} - join without read permissions on universe".format(status)) >>>   rows = list(server.iproto.py_con.subscribe(cluster_uuid, >>> replica_uuid)) >>> -print len(rows) == 1 and rows[0].return_message.find('Read access') >>> >= 0 and \ >>> -    'ok' or 'not ok', '-', 'subscribe without read permissions on >>> universe' >>> +status = len(rows) == 1 and rows[0].return_message.find("Read >>> access") >= 0 and \ >>> +        "ok" or "not ok" >>> +print("{} - subscribe without read permissions on >>> universe".format(status)) >>>   ## Write permission to space `_cluster` is required to perform JOIN >>>   server.admin("box.schema.user.grant('guest', 'read', 'universe')") >>>   server.iproto.reconnect() # re-connect with new permissions >>>   rows = list(server.iproto.py_con.join(replica_uuid)) >>> -print len(rows) == 1 and rows[0].return_message.find('Write access') >>> >= 0 and \ >>> -    'ok' or 'not ok', '-', 'join without write permissions to _cluster' >>> +status = len(rows) == 1 and rows[0].return_message.find("Write >>> access") >= 0 and \ >>> +        "ok" or "not ok"> @@ -235,16 +240,16 @@ try: >>>   except Exception as e: >>>       line = "ER_READONLY" >>>       if failed.logfile_pos.seek_once(line) >= 0: >>> -        print "'%s' exists in server log" % line >>> +        print("\'{}\' exists in server log".format(line)) >> 10. You don't need to escape ' when they are inside of ". > --- a/test/replication-py/cluster.test.py > +++ b/test/replication-py/cluster.test.py > @@ -240,7 +240,7 @@ try: >  except Exception as e: >      line = "ER_READONLY" >      if failed.logfile_pos.seek_once(line) >= 0: > -        print("\'{}\' exists in server log".format(line)) > +        print("'{}' exists in server log".format(line)) > >  master.admin("box.cfg { read_only = false }") >> >>> -master.admin('box.cfg { read_only = false }') >>> +master.admin("box.cfg { read_only = false }") >>> -print '-------------------------------------------------------------' >>> -print 'JOIN replica with different replica set UUID' >>> -print '-------------------------------------------------------------' >>> +print("-------------------------------------------------------------") >>> +print("JOIN replica with different replica set UUID") >>> +print("-------------------------------------------------------------") >>>   failed = TarantoolServer(server.ini) >>> -failed.script = 'replication-py/uuid_mismatch.lua' >>> +failed.script = "replication-py/uuid_mismatch.lua" >>>   failed.vardir = server.vardir >>>   failed.rpl_master = master >>>   failed.name = "uuid_mismatch" >>> @@ -254,15 +259,15 @@ try: >>>   except Exception as e: >>>       line = "ER_REPLICASET_UUID_MISMATCH" >>>       if failed.logfile_pos.seek_once(line) >= 0: >>> -        print "'%s' exists in server log" % line >>> +        print("\'{}\' exists in server log".format(line)) >> 11. You don't need to escape ' when they are inside of ". > > Right. > > @@ -259,7 +259,7 @@ try: >  except Exception as e: >      line = "ER_REPLICASET_UUID_MISMATCH" >      if failed.logfile_pos.seek_once(line) >= 0: > -        print("\'{}\' exists in server log".format(line)) > +        print("'{}' exists in server log".format(line)) > >  failed.cleanup() > >> >> 12. There is also one place, where you removed an empty line in the >> end of file. I can't find it again, but it belongs to the last >> commit. Please, move it. > > Done, it was xlog-py/dup_key.test.py. > > > 1. > https://lists.tarantool.org/pipermail/tarantool-patches/2020-December/021323.html > > > 2. > https://lists.tarantool.org/pipermail/tarantool-patches/2020-December/021044.html > >