Re: [PATCH 2/2] python: Fix style issues - sanlock-devel - Fedora mailing-lists

16 Jun 2019

On Sun, Jun 16, 2019 at 4:11 PM Pavel Bar pbar@redhat.com wrote:
...
On Fri, Jun 14, 2019 at 8:15 PM Nir Soffer nirsof@gmail.com wrote:
...
Fix style issues:

2 blank lines around functions and classes
classes at the top of the module
reformat to avoid long hard to read lines
whitespace before ','
whitespace after #
unused imports
remove blank line at end of file

Signed-off-by: Nir Soffer nsoffer@redhat.com
python/example.py    | 28 +++++++++++++++++++--------
 tests/conftest.py    |  8 ++++----
 tests/python_test.py | 46 ++++++++++++++++++++++++++++++--------------
 3 files changed, 56 insertions(+), 26 deletions(-)

diff --git a/python/example.py b/python/example.py
index 6ef9f7d..1be7e66 100644
--- a/python/example.py
+++ b/python/example.py
@@ -8,41 +8,46 @@ import sanlock
HOST_ID = 1
 LOCKSPACE_NAME = "lockspace1"
 RESOURCE_NAME = "resource1"



def sigTermHandler():
     print "SIGTERM signal received"



def main():
     signal.signal(signal.SIGTERM, sigTermHandler)
 print "Creating the sanlock disk"
 fd, disk = tempfile.mkstemp()
 os.close(fd)



os.chown(disk, pwd.getpwnam("sanlock").pw_uid,

grp.getgrnam("sanlock").gr_gid)

os.chown(
   disk, pwd.getpwnam("sanlock").pw_uid,



grp.getgrnam("sanlock").gr_gid)
Not sure that the 2nd style is better, IMHO, it's even worse.
The same long line with lot's of parameters with/without explicitly naming
the receiving parameter's name.
In my opinion if doing such a cleanup/improvement work, it should't be
just "to mark a V" that some tool is not complaining anymore.
In this case I believe it's better to split the line, so every parameter
will appear in a single line as it really adds to readability. That's what
you did below with "*owners = sanlock.read_resource_owners*..."
Using one argument per for all calls does not improve readability, so we
need a simple rule when
to more to one argument per line style.
My general indentation rules are:
1. use one line if it fits in one line
2. or move arguments to next line if it fits in 2 lines
3. or use one line per argument
This case matches rule 2.
offset = sanlock.get_alignment(disk)
...
...
 SNLK_DISKS = [(disk, offset)]

 print "Registering to sanlock"
 fd = sanlock.register()

 print "Initializing '%s'" % (LOCKSPACE_NAME,)


sanlock.write_lockspace(LOCKSPACE_NAME, disk, max_hosts=0,

iotimeout=0, align=1048576, sector=512)

sanlock.write_lockspace(LOCKSPACE_NAME, disk, align=1048576,

sector=512)
 print "Initializing '%s' on '%s'" % (RESOURCE_NAME, LOCKSPACE_NAME)


sanlock.write_resource(LOCKSPACE_NAME, RESOURCE_NAME, SNLK_DISKS,

align=1048576, sector=512)

sanlock.write_resource(
   LOCKSPACE_NAME, RESOURCE_NAME, SNLK_DISKS, align=1048576,



sector=512)
Same note as above.
...
 print "Acquiring the id '%i' on '%s'" % (HOST_ID, LOCKSPACE_NAME)
 sanlock.add_lockspace(LOCKSPACE_NAME, HOST_ID, disk)

 try:
     print "Acquiring '%s' on '%s'" % (RESOURCE_NAME, LOCKSPACE_NAME)


   sanlock.acquire(LOCKSPACE_NAME, RESOURCE_NAME, SNLK_DISKS,



slkfd=fd,

                   version=0)




   sanlock.acquire(


       LOCKSPACE_NAME, RESOURCE_NAME, SNLK_DISKS, slkfd=fd,



version=0)

   while True:
       print "Trying to get lockspace '%s' hosts" % LOCKSPACE_NAME
       try:
           hosts_list = sanlock.get_hosts(LOCKSPACE_NAME)
       except sanlock.SanlockException as e:



@@ -50,13 +55,19 @@ def main():
                     raise
             else:
                 print "Lockspace '%s' hosts: " % LOCKSPACE_NAME,
hosts_list
                 break
             time.sleep(5)

   print "Resource '%s' owners: " % RESOURCE_NAME, \


       sanlock.read_resource_owners(


           LOCKSPACE_NAME, RESOURCE_NAME, SNLK_DISKS,



align=1048576, sector=512)


   owners = sanlock.read_resource_owners(


       LOCKSPACE_NAME,


       RESOURCE_NAME,


       SNLK_DISKS,


       align=1048576,


       sector=512)


   print "Resource '%s' owners: %s" % (RESOURCE_NAME, owners)




Doesn't the "print " require "()" in Python 3? Don't we want to be Python
3 compatible (at least if we "touch" some code)?
Yes, the example code is not python 3 compatible yet. Converting it to
python 3 require
additional work, but it is not easy as this example is not easy to test.
+
...
...
     print "Releasing '%s' on '%s'" % (RESOURCE_NAME, LOCKSPACE_NAME)
     sanlock.release(LOCKSPACE_NAME, RESOURCE_NAME, SNLK_DISKS,

slkfd=fd)
     except Exception as e:
         print "Exception: ", e
     finally:
@@ -64,7 +75,8 @@ def main():
         sanlock.rem_lockspace(LOCKSPACE_NAME, HOST_ID, disk)
 print "Removing the sanlock disk"
 os.remove(disk)





if __name__ == '__main__':
     main()
diff --git a/tests/conftest.py b/tests/conftest.py
index 70e83d4..868ae46 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -2,18 +2,21 @@
 Fixtures for sanlock testing.
 """
 from __future__ import absolute_import
import os
-import stat
import pytest
from . import storage
 from . import util
+class SanlockIsRunning(Exception):

""" Raised if sanlock running when it should not """



@pytest.fixture
 def sanlock_daemon():
     """
     Run sanlock daemon during a test.
     """
@@ -48,8 +51,5 @@ def user_4k_path(request):
@pytest.fixture
 def no_sanlock_daemon():
     if util.sanlock_is_running():
         raise SanlockIsRunning



-class SanlockIsRunning(Exception):

pass

diff --git a/tests/python_test.py b/tests/python_test.py
index 3242f57..f37eab4 100644
--- a/tests/python_test.py
+++ b/tests/python_test.py
@@ -34,11 +34,11 @@ ALIGNMENT_2M = 2 * MiB
 SECTOR_SIZE_512 = 512
 SECTOR_SIZE_4K = 4 * KiB
FILE_NAMES = [

#name, encoding


# name, encoding
("ascii", None),
(u"ascii", None),
(u"\u05d0", None),
(u"\u05d0", "utf-8"),

]
@@ -47,25 +47,31 @@ LOCKSPACE_OR_RESOURCE_NAMES = [
     # Bytes are supported with python 2 and 3.
     pytest.param(b"\xd7\x90"),
     # Python 2 also supports str.
     pytest.param(
         "\xd7\x90",

   marks=pytest.mark.skipif(six.PY3, reason="python 3 supports only



bytes")),

   marks=pytest.mark.skipif(


       six.PY3,


       reason="python 3 supports only bytes")),

# Python 2 also supports unicode with ascii content.
   pytest.param(
       u"ascii",


   marks=pytest.mark.skipif(six.PY3, reason="python 3 supports only



bytes")),

   marks=pytest.mark.skipif(


       six.PY3,


       reason="python 3 supports only bytes")),



]
-@pytest.mark.parametrize("filename, encoding" , FILE_NAMES)
-@pytest.mark.parametrize("size,offset", [



+@pytest.mark.parametrize("filename, encoding", FILE_NAMES)
+@pytest.mark.parametrize("size, offset", [
     # Smallest offset.
     (LOCKSPACE_SIZE, 0),
     # Large offset.
     (LARGE_FILE_SIZE, LARGE_FILE_SIZE - LOCKSPACE_SIZE),
 ])
-def test_write_lockspace(tmpdir, sanlock_daemon, filename, encoding,
size, offset):
+def test_write_lockspace(

   tmpdir, sanlock_daemon, filename, encoding, size, offset):

path = util.generate_path(tmpdir, filename, encoding)
   util.create_file(path, size)
# Test read and write with default alignment and sector size values.
   sanlock.write_lockspace(b"ls_name", path, offset=offset, iotimeout=1)


@@ -104,11 +110,15 @@ def test_write_lockspace_4k(user_4k_path,
sanlock_daemon, align):
     with io.open(user_4k_path, "rb+") as f:
         f.write(align * b"x")
     util.write_guard(user_4k_path, align)
 sanlock.write_lockspace(


   b"ls_name", user_4k_path, iotimeout=1, align=align,



sector=SECTOR_SIZE_4K)

   b"ls_name",


   user_4k_path,


   iotimeout=1,


   align=align,


   sector=SECTOR_SIZE_4K)


ls = sanlock.read_lockspace(
       user_4k_path, align=align, sector=SECTOR_SIZE_4K)
assert ls == {"iotimeout": 1, "lockspace": b"ls_name"}


@@ -146,11 +156,12 @@ def
test_read_lockspace_4k_invalid_sector_size(sanlock_daemon, user_4k_path):
     # Smallest offset.
     (MIN_RES_SIZE, 0),
     # Large offset.
     (LARGE_FILE_SIZE, LARGE_FILE_SIZE - MIN_RES_SIZE),
 ])
-def test_write_resource(tmpdir, sanlock_daemon, filename, encoding,
size, offset):
+def test_write_resource(

   tmpdir, sanlock_daemon, filename, encoding, size, offset):

path = util.generate_path(tmpdir, filename, encoding)
   util.create_file(path, size)
   disks = [(path, offset)]
# Test read and write with default alignment and sector size values.


@@ -525,19 +536,21 @@ def
test_add_lockspace_parse_args(no_sanlock_daemon, name, filename, encoding):
         sanlock.add_lockspace(name, 1, path, 0, wait=False)
@pytest.mark.parametrize("name", LOCKSPACE_OR_RESOURCE_NAMES)
 @pytest.mark.parametrize("filename,encoding", FILE_NAMES)
-def test_write_lockspace_parse_args(no_sanlock_daemon, name, filename,
encoding):
+def test_write_lockspace_parse_args(

   no_sanlock_daemon, name, filename, encoding):

path = util.generate_path("/tmp/", filename, encoding)
   with raises_sanlock_errno():
       sanlock.write_lockspace(name, path)

@pytest.mark.parametrize("name", LOCKSPACE_OR_RESOURCE_NAMES)
 @pytest.mark.parametrize("filename,encoding", FILE_NAMES)
-def test_write_resource_parse_args(no_sanlock_daemon, name, filename,
encoding):
+def test_write_resource_parse_args(

   no_sanlock_daemon, name, filename, encoding):

path = util.generate_path("/tmp/", filename, encoding)
   disks = [(path, 0)]
   with raises_sanlock_errno():
       sanlock.write_resource(name, b"res_name", disks)

@@ -545,11 +558,12 @@ def
test_write_resource_parse_args(no_sanlock_daemon, name, filename, encoding):
         sanlock.write_resource(b"ls_name", name, disks)
@pytest.mark.parametrize("name", LOCKSPACE_OR_RESOURCE_NAMES)
 @pytest.mark.parametrize("filename,encoding", FILE_NAMES)
-def test_release_resource_parse_args(no_sanlock_daemon, name, filename,
encoding):
+def test_release_resource_parse_args(

   no_sanlock_daemon, name, filename, encoding):

path = util.generate_path("/tmp/", filename, encoding)
   disks = [(path, 0)]
   with raises_sanlock_errno():
       sanlock.release(name, b"res_name", disks)

@@ -557,11 +571,12 @@ def
test_release_resource_parse_args(no_sanlock_daemon, name, filename, encoding
         sanlock.release(b"ls_name", name, disks)
@pytest.mark.parametrize("name", LOCKSPACE_OR_RESOURCE_NAMES)
 @pytest.mark.parametrize("filename,encoding", FILE_NAMES)
-def test_read_resource_owners_parse_args(no_sanlock_daemon, name,
filename, encoding):
+def test_read_resource_owners_parse_args(

   no_sanlock_daemon, name, filename, encoding):

path = util.generate_path("/tmp/", filename, encoding)
   disks = [(path, 0)]
   with raises_sanlock_errno():
       sanlock.read_resource_owners(name, b"res_name", disks)

@@ -601,11 +616,12 @@ def test_set_event_parse_args(no_sanlock_daemon,
name):
         sanlock.set_event(name, 1, 1, 1)
@pytest.mark.parametrize("name", LOCKSPACE_OR_RESOURCE_NAMES)
 @pytest.mark.parametrize("filename,encoding", FILE_NAMES)
-def test_init_lockspace_parse_args(no_sanlock_daemon, name, filename,
encoding):
+def test_init_lockspace_parse_args(

   no_sanlock_daemon, name, filename, encoding):

path = util.generate_path("/tmp/", filename, encoding)
   with raises_sanlock_errno(errno.ENODEV):
       sanlock.init_lockspace(name, path)

@@ -617,23 +633,25 @@ def
test_init_resource_parse_args(no_sanlock_daemon, name, filename, encoding):
     with raises_sanlock_errno(errno.ENOENT):
         sanlock.init_resource(b"ls_name", name, disks)
     with raises_sanlock_errno(errno.ENOENT):
         sanlock.init_resource(name, b"res_name", disks)



@pytest.mark.parametrize("filename,encoding", FILE_NAMES)
 def test_get_alignment_parse_args(no_sanlock_daemon, filename, encoding):
     path = util.generate_path("/tmp/", filename, encoding)
     with raises_sanlock_errno(errno.ENOENT):
         sanlock.get_alignment(path)



@pytest.mark.parametrize("filename,encoding", FILE_NAMES)
 def test_read_lockspace_parse_args(no_sanlock_daemon, filename,
encoding):
     path = util.generate_path("/tmp/", filename, encoding)
     with raises_sanlock_errno():
         sanlock.read_lockspace(path)



@pytest.mark.parametrize("filename,encoding", FILE_NAMES)
 def test_read_resource_parse_args(no_sanlock_daemon, filename, encoding):
     path = util.generate_path("/tmp/", filename, encoding)
     with raises_sanlock_errno():
         sanlock.read_resource(path)



--
2.17.2

    