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..."

     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)?

+
         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