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 7A19545C304 for ; Sun, 13 Dec 2020 20:58:17 +0300 (MSK) References: From: Vladislav Shpilevoy Message-ID: <6265c0f8-3b96-04db-0bcd-6bb77dc9fe71@tarantool.org> Date: Sun, 13 Dec 2020 18:58:15 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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: sergeyb@tarantool.org, tarantool-patches@dev.tarantool.org, imun@tarantool.org Cc: alexander.turenko@tarantool.org 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. 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? > - 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. > + > 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. > 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. > 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? > --- > - - [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? > --- > ... > 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. I suspect this has something to do with the second commit, and therefore belongs to it. > + 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 and rows[0].return_message.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? > +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 ". > -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 ". 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.