open-iscsi / targetd

Remote configuration of a LIO-based storage appliance
GNU General Public License v3.0
71 stars 28 forks source link

Standardize code style #82

Closed johanfleury closed 3 years ago

johanfleury commented 3 years ago

While working on #81 I saw that their seems to be no standardized coding style throughout the project.

Here are a few examples of things I think could be fixed:

$ grep -c \' **/*.py
setup.py:10
targetd/backends/btrfs.py:19
targetd/backends/__init__.py:0
targetd/backends/lvm.py:14
targetd/backends/zfs.py:23
targetd/block.py:61
targetd/fs.py:15
targetd/__init__.py:0
targetd/main.py:38
targetd/mount.py:2
targetd/nfs.py:32
targetd/utils.py:3
test/__init__.py:0
test/targetd_test.py:16
test/testlib.py:8

$ grep -c \" **/*.py
setup.py:0
targetd/backends/btrfs.py:14
targetd/backends/__init__.py:0
targetd/backends/lvm.py:38
targetd/backends/zfs.py:109
targetd/block.py:40
targetd/fs.py:20
targetd/__init__.py:0
targetd/main.py:50
targetd/mount.py:4
targetd/nfs.py:43
targetd/utils.py:4
test/__init__.py:0
test/targetd_test.py:125
test/testlib.py:17
$ grep -c '.\{79,\}' **/*.py
setup.py:0
targetd/backends/btrfs.py:6
targetd/backends/__init__.py:0
targetd/backends/lvm.py:2
targetd/backends/zfs.py:34
targetd/block.py:9
targetd/fs.py:14
targetd/__init__.py:0
targetd/main.py:5
targetd/mount.py:1
targetd/nfs.py:5
targetd/utils.py:1
test/__init__.py:0
test/targetd_test.py:11
test/testlib.py:1

$ grep -c '.\{99,\}' **/*.py
setup.py:0
targetd/backends/btrfs.py:0
targetd/backends/__init__.py:0
targetd/backends/lvm.py:0
targetd/backends/zfs.py:14
targetd/block.py:6
targetd/fs.py:6
targetd/__init__.py:0
targetd/main.py:0
targetd/mount.py:0
targetd/nfs.py:0
targetd/utils.py:0
test/__init__.py:0
test/targetd_test.py:0
test/testlib.py:0
                raise TargetdError(TargetdError.INVALID_ARGUMENT,
                                   "Size %d need a larger than size in original volume %s in pool %s" % (size,
                                                                                                         vol_orig,
                                                                                                         pool))

What I would like to suggest is using something like black (which is an equivalent to go fmt) to standardize the code style. black has a test mode that can be used in CI to test that a PR is well formated.

One is not encouraged to derive from black’s default configuration, but it can still be tweaked (to some extent) with some settings in pyproject.toml.

Given the previous example, running black --check --diff -t py36 would do something like that:

-                raise TargetdError(TargetdError.INVALID_ARGUMENT,
-                                   "Size %d need a larger than size in original volume %s in pool %s" % (size,
-                                                                                                         vol_orig,
-                                                                                                         pool))
+                raise TargetdError(
+                    TargetdError.INVALID_ARGUMENT,
+                    "Size %d need a larger than size in original volume %s in pool %s"
+                    % (size, vol_orig, pool),
+                )

Regarding string formatting, I would suggest to drop %-formatted string and use either the new f-strings if there’s no intent to be compatible with Python versions older that 3.6 (2016) or the str.format if compatibility with older version is a issue.

I can propose a PR if you’re ok with this proposal.

tasleson commented 3 years ago

Yes I support this. Please take a look at all the options, eg. yapf too, for a comparison. One thing that would be good is if whatever format tool is utilized that it's consistent from release to release, so that people don't need the exact same version to produce the same results.

johanfleury commented 3 years ago

Please take a look at all the options, eg. yapf too, for a comparison

I only use black and never tested yapf.

I think yapf has more customization option (which can be good or bad depending on the context), but in the end they achieve the same goal. On the other hand, I think black has a bigger popularity (but I don’t have any metrics to support that statement).

It also seems that yapf doesn’t deal with standardizing quotes.

I found this article that describes their behaviors in details: Auto formatters for Python 👨‍💻🤖.

One thing that would be good is if whatever format tool is utilized that it's consistent from release to release, so that people don't need the exact same version to produce the same results.

I’m using black for quite some time now and never noticed big changes induced by new release.


In the end I think this is your call to make. I have a preference for black, but not a strong opinion on this.