On Sun, Jun 16, 2019 at 4:11 PM Pavel Bar <pbar(a)redhat.com> wrote:
>
>
> On Fri, Jun 14, 2019 at 8:15 PM Nir Soffer <nirsof(a)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(a)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")),
>> ]
>>
>> -(a)pytest.mark.parametrize("filename, encoding" , FILE_NAMES)
>> -(a)pytest.mark.parametrize("size,offset", [
>> +
>> +(a)pytest.mark.parametrize("filename, encoding", FILE_NAMES)
>> +(a)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
>>
>>