From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 E67D445C304 for ; Mon, 14 Dec 2020 14:44:21 +0300 (MSK) References: <6265c0f8-3b96-04db-0bcd-6bb77dc9fe71@tarantool.org> From: Sergey Bronnikov Message-ID: <31bded2b-32e0-5b65-92d1-9ef08415d3fc@tarantool.org> Date: Mon, 14 Dec 2020 14:44:20 +0300 MIME-Version: 1.0 In-Reply-To: <6265c0f8-3b96-04db-0bcd-6bb77dc9fe71@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US 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: Vladislav Shpilevoy , tarantool-patches@dev.tarantool.org Cc: alexander.turenko@tarantool.org 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