Re: [PATCH 2/2] python: Fix style issues

[Nir Soffer]

On Sun, Jun 16, 2019 at 4:11 PM Pavel Bar <pbar(a)redhat.com&gt; wrote:

>  On Fri, Jun 14, 2019 at 8:15 PM Nir Soffer <nirsof(a)gmail.com&gt; 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(a)redhat.com&gt;
> > ---
> >  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")),
> >  ]
> >
> > -(a)pytest.mark.parametrize(&quot;filename, encoding" , FILE_NAMES)
> > -(a)pytest.mark.parametrize(&quot;size,offset&quot;, [
> > +
> > +(a)pytest.mark.parametrize(&quot;filename, encoding", FILE_NAMES)
> > +(a)pytest.mark.parametrize(&quot;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
> >
> >