79 is the PEP8 recommendation. This recommendation works well for
reading patch diffs in TUI email clients.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/iotests.py | 69 ++++++++++++++++++++++-------------
 tests/qemu-iotests/pylintrc   |  6 ++-
 2 files changed, 48 insertions(+), 27 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 54d68774e1..1be11f491f 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -76,9 +76,11 @@
 def qemu_img(*args):
     '''Run qemu-img and return the exit code'''
     devnull = open('/dev/null', 'r+')
-    exitcode = subprocess.call(qemu_img_args + list(args), stdin=devnull, stdout=devnull)
+    exitcode = subprocess.call(qemu_img_args + list(args),
+                               stdin=devnull, stdout=devnull)
     if exitcode < 0:
-        sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' '.join(qemu_img_args + list(args))))
+        sys.stderr.write('qemu-img received signal %i: %s\n' % (
+            -exitcode, ' '.join(qemu_img_args + list(args))))
     return exitcode
 
 def ordered_qmp(qmsg, conv_keys=True):
@@ -117,7 +119,8 @@ def qemu_img_verbose(*args):
     '''Run qemu-img without suppressing its output and return the exit code'''
     exitcode = subprocess.call(qemu_img_args + list(args))
     if exitcode < 0:
-        sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' '.join(qemu_img_args + list(args))))
+        sys.stderr.write('qemu-img received signal %i: %s\n' % (
+            -exitcode, ' '.join(qemu_img_args + list(args))))
     return exitcode
 
 def qemu_img_pipe(*args):
@@ -128,7 +131,8 @@ def qemu_img_pipe(*args):
                             universal_newlines=True)
     exitcode = subp.wait()
     if exitcode < 0:
-        sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' '.join(qemu_img_args + list(args))))
+        sys.stderr.write('qemu-img received signal %i: %s\n' % (
+            -exitcode, ' '.join(qemu_img_args + list(args))))
     return subp.communicate()[0]
 
 def qemu_img_log(*args):
@@ -158,7 +162,8 @@ def qemu_io(*args):
                             universal_newlines=True)
     exitcode = subp.wait()
     if exitcode < 0:
-        sys.stderr.write('qemu-io received signal %i: %s\n' % (-exitcode, ' '.join(args)))
+        sys.stderr.write('qemu-io received signal %i: %s\n' % (
+            -exitcode, ' '.join(args)))
     return subp.communicate()[0]
 
 def qemu_io_log(*args):
@@ -280,10 +285,13 @@ def filter_test_dir(msg):
 def filter_win32(msg):
     return win32_re.sub("", msg)
 
-qemu_io_re = re.compile(r"[0-9]* ops; [0-9\/:. sec]* \([0-9\/.inf]* [EPTGMKiBbytes]*\/sec and [0-9\/.inf]* ops\/sec\)")
+qemu_io_re = re.compile(r"[0-9]* ops; [0-9\/:. sec]* "
+                        r"\([0-9\/.inf]* [EPTGMKiBbytes]*\/sec "
+                        r"and [0-9\/.inf]* ops\/sec\)")
 def filter_qemu_io(msg):
     msg = filter_win32(msg)
-    return qemu_io_re.sub("X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)", msg)
+    return qemu_io_re.sub("X ops; XX:XX:XX.X "
+                          "(XXX YYY/sec and XXX ops/sec)", msg)
 
 chown_re = re.compile(r"chown [0-9]+:[0-9]+")
 def filter_chown(msg):
@@ -335,7 +343,9 @@ def filter_img_info(output, filename):
         line = line.replace(filename, 'TEST_IMG') \
                    .replace(imgfmt, 'IMGFMT')
         line = re.sub('iters: [0-9]+', 'iters: XXX', line)
-        line = re.sub('uuid: [-a-f0-9]+', 'uuid: XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX', line)
+        line = re.sub('uuid: [-a-f0-9]+',
+                      'uuid: XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX',
+                      line)
         line = re.sub('cid: [0-9]+', 'cid: XXXXXXXXXX', line)
         lines.append(line)
     return '\n'.join(lines)
@@ -356,12 +366,9 @@ def log(msg, filters=(), indent=None):
     for flt in filters:
         msg = flt(msg)
     if isinstance(msg, (dict, list)):
-        # Python < 3.4 needs to know not to add whitespace when pretty-printing:
-        separators = (', ', ': ') if indent is None else (',', ': ')
         # Don't sort if it's already sorted
         do_sort = not isinstance(msg, OrderedDict)
-        print(json.dumps(msg, sort_keys=do_sort,
-                         indent=indent, separators=separators))
+        print(json.dumps(msg, sort_keys=do_sort, indent=indent))
     else:
         print(msg)
 
@@ -529,11 +536,13 @@ def pause_drive(self, drive, event=None):
             self.pause_drive(drive, "write_aio")
             return
         self.qmp('human-monitor-command',
-                 command_line='qemu-io %s "break %s bp_%s"' % (drive, event, drive))
+                 command_line='qemu-io %s "break %s bp_%s"' % (
+                     drive, event, drive))
 
     def resume_drive(self, drive):
         self.qmp('human-monitor-command',
-                 command_line='qemu-io %s "remove_break bp_%s"' % (drive, drive))
+                 command_line='qemu-io %s "remove_break bp_%s"' % (
+                     drive, drive))
 
     def hmp_qemu_io(self, drive, cmd):
         '''Write to a given drive using an HMP command'''
@@ -790,16 +799,18 @@ def dictpath(self, d, path):
                 idx = int(idx)
 
             if not isinstance(d, dict) or component not in d:
-                self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
+                self.fail(f'failed path traversal for "{path}" in "{d}"')
             d = d[component]
 
             if m:
                 if not isinstance(d, list):
-                    self.fail('path component "%s" in "%s" is not a list in "%s"' % (component, path, str(d)))
+                    self.fail(f'path component "{component}" in "{path}" '
+                              f'is not a list in "{d}"')
                 try:
                     d = d[idx]
                 except IndexError:
-                    self.fail('invalid index "%s" in path "%s" in "%s"' % (idx, path, str(d)))
+                    self.fail(f'invalid index "{idx}" in path "{path}" '
+                              f'in "{d}"')
         return d
 
     def assert_qmp_absent(self, d, path):
@@ -850,10 +861,13 @@ def assert_json_filename_equal(self, json_filename, reference):
         '''Asserts that the given filename is a json: filename and that its
            content is equal to the given reference object'''
         self.assertEqual(json_filename[:5], 'json:')
-        self.assertEqual(self.vm.flatten_qmp_object(json.loads(json_filename[5:])),
-                         self.vm.flatten_qmp_object(reference))
+        self.assertEqual(
+            self.vm.flatten_qmp_object(json.loads(json_filename[5:])),
+            self.vm.flatten_qmp_object(reference)
+        )
 
-    def cancel_and_wait(self, drive='drive0', force=False, resume=False, wait=60.0):
+    def cancel_and_wait(self, drive='drive0', force=False,
+                        resume=False, wait=60.0):
         '''Cancel a block job and wait for it to finish, returning the event'''
         result = self.vm.qmp('block-job-cancel', device=drive, force=force)
         self.assert_qmp(result, 'return', {})
@@ -877,8 +891,8 @@ def cancel_and_wait(self, drive='drive0', force=False, resume=False, wait=60.0):
         self.assert_no_active_block_jobs()
         return result
 
-    def wait_until_completed(self, drive='drive0', check_offset=True, wait=60.0,
-                             error=None):
+    def wait_until_completed(self, drive='drive0', check_offset=True,
+                             wait=60.0, error=None):
         '''Wait for a block job to finish, returning the event'''
         while True:
             for event in self.vm.get_qmp_events(wait=wait):
@@ -1017,8 +1031,11 @@ def verify_quorum():
         notrun('quorum support missing')
 
 def qemu_pipe(*args):
-    '''Run qemu with an option to print something and exit (e.g. a help option),
-    and return its output'''
+    """
+    Run qemu with an option to print something and exit (e.g. a help option).
+
+    :return: QEMU's stdout output.
+    """
     args = [qemu_prog] + qemu_opts + list(args)
     subp = subprocess.Popen(args, stdout=subprocess.PIPE,
                             stderr=subprocess.STDOUT,
@@ -1056,8 +1073,8 @@ def func_wrapper(test_case: QMPTestCase, *args, **kwargs):
 
             usf_list = list(set(fmts) - set(supported_formats(read_only)))
             if usf_list:
-                test_case.case_skip('{}: formats {} are not whitelisted'.format(
-                    test_case, usf_list))
+                test_case.case_skip(f'{test_case}: formats {usf_list} '
+                                    'are not whitelisted')
                 return None
             else:
                 return func(test_case, *args, **kwargs)
diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc
index 8720b6a0de..8d02f00607 100644
--- a/tests/qemu-iotests/pylintrc
+++ b/tests/qemu-iotests/pylintrc
@@ -19,4 +19,8 @@ disable=invalid-name,
         too-many-public-methods,
         # These are temporary, and should be removed:
         missing-docstring,
-        line-too-long,
+
+[FORMAT]
+
+# Maximum number of characters on a single line.
+max-line-length=79
-- 
2.21.1

On 3/4/20 10:38 PM, John Snow wrote:
> 79 is the PEP8 recommendation. This recommendation works well for
> reading patch diffs in TUI email clients.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   tests/qemu-iotests/iotests.py | 69 ++++++++++++++++++++++-------------
>   tests/qemu-iotests/pylintrc   |  6 ++-
>   2 files changed, 48 insertions(+), 27 deletions(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 54d68774e1..1be11f491f 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -76,9 +76,11 @@
>   def qemu_img(*args):
>       '''Run qemu-img and return the exit code'''
>       devnull = open('/dev/null', 'r+')
> -    exitcode = subprocess.call(qemu_img_args + list(args), stdin=devnull, stdout=devnull)
> +    exitcode = subprocess.call(qemu_img_args + list(args),
> +                               stdin=devnull, stdout=devnull)
>       if exitcode < 0:
> -        sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' '.join(qemu_img_args + list(args))))
> +        sys.stderr.write('qemu-img received signal %i: %s\n' % (
> +            -exitcode, ' '.join(qemu_img_args + list(args))))

Do we want to indent Python like C and align argument below opening 
parenthesis? Except when using sys.stderr.write() you seem to do it.

>       return exitcode
>   
>   def ordered_qmp(qmsg, conv_keys=True):
> @@ -117,7 +119,8 @@ def qemu_img_verbose(*args):
>       '''Run qemu-img without suppressing its output and return the exit code'''
>       exitcode = subprocess.call(qemu_img_args + list(args))
>       if exitcode < 0:
> -        sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' '.join(qemu_img_args + list(args))))
> +        sys.stderr.write('qemu-img received signal %i: %s\n' % (
> +            -exitcode, ' '.join(qemu_img_args + list(args))))
>       return exitcode
>   
>   def qemu_img_pipe(*args):
> @@ -128,7 +131,8 @@ def qemu_img_pipe(*args):
>                               universal_newlines=True)
>       exitcode = subp.wait()
>       if exitcode < 0:
> -        sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' '.join(qemu_img_args + list(args))))
> +        sys.stderr.write('qemu-img received signal %i: %s\n' % (
> +            -exitcode, ' '.join(qemu_img_args + list(args))))
>       return subp.communicate()[0]
>   
>   def qemu_img_log(*args):
> @@ -158,7 +162,8 @@ def qemu_io(*args):
>                               universal_newlines=True)
>       exitcode = subp.wait()
>       if exitcode < 0:
> -        sys.stderr.write('qemu-io received signal %i: %s\n' % (-exitcode, ' '.join(args)))
> +        sys.stderr.write('qemu-io received signal %i: %s\n' % (
> +            -exitcode, ' '.join(args)))
>       return subp.communicate()[0]
>   
>   def qemu_io_log(*args):
> @@ -280,10 +285,13 @@ def filter_test_dir(msg):
>   def filter_win32(msg):
>       return win32_re.sub("", msg)
>   
> -qemu_io_re = re.compile(r"[0-9]* ops; [0-9\/:. sec]* \([0-9\/.inf]* [EPTGMKiBbytes]*\/sec and [0-9\/.inf]* ops\/sec\)")
> +qemu_io_re = re.compile(r"[0-9]* ops; [0-9\/:. sec]* "
> +                        r"\([0-9\/.inf]* [EPTGMKiBbytes]*\/sec "
> +                        r"and [0-9\/.inf]* ops\/sec\)")
>   def filter_qemu_io(msg):
>       msg = filter_win32(msg)
> -    return qemu_io_re.sub("X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)", msg)
> +    return qemu_io_re.sub("X ops; XX:XX:XX.X "
> +                          "(XXX YYY/sec and XXX ops/sec)", msg)
>   
>   chown_re = re.compile(r"chown [0-9]+:[0-9]+")
>   def filter_chown(msg):
> @@ -335,7 +343,9 @@ def filter_img_info(output, filename):
>           line = line.replace(filename, 'TEST_IMG') \
>                      .replace(imgfmt, 'IMGFMT')
>           line = re.sub('iters: [0-9]+', 'iters: XXX', line)
> -        line = re.sub('uuid: [-a-f0-9]+', 'uuid: XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX', line)
> +        line = re.sub('uuid: [-a-f0-9]+',
> +                      'uuid: XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX',
> +                      line)
>           line = re.sub('cid: [0-9]+', 'cid: XXXXXXXXXX', line)
>           lines.append(line)
>       return '\n'.join(lines)
> @@ -356,12 +366,9 @@ def log(msg, filters=(), indent=None):
>       for flt in filters:
>           msg = flt(msg)
>       if isinstance(msg, (dict, list)):
> -        # Python < 3.4 needs to know not to add whitespace when pretty-printing:
> -        separators = (', ', ': ') if indent is None else (',', ': ')
>           # Don't sort if it's already sorted
>           do_sort = not isinstance(msg, OrderedDict)
> -        print(json.dumps(msg, sort_keys=do_sort,
> -                         indent=indent, separators=separators))
> +        print(json.dumps(msg, sort_keys=do_sort, indent=indent))

Unrelated change. Maybe worth a separate patch?

>       else:
>           print(msg)
>   
> @@ -529,11 +536,13 @@ def pause_drive(self, drive, event=None):
>               self.pause_drive(drive, "write_aio")
>               return
>           self.qmp('human-monitor-command',
> -                 command_line='qemu-io %s "break %s bp_%s"' % (drive, event, drive))
> +                 command_line='qemu-io %s "break %s bp_%s"' % (
> +                     drive, event, drive))
>   
>       def resume_drive(self, drive):
>           self.qmp('human-monitor-command',
> -                 command_line='qemu-io %s "remove_break bp_%s"' % (drive, drive))
> +                 command_line='qemu-io %s "remove_break bp_%s"' % (
> +                     drive, drive))

Can this work?

                     command_line='qemu-io %s "remove_break bp_%s"'
                                  % (drive, drive))

Regardless:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>   
>       def hmp_qemu_io(self, drive, cmd):
>           '''Write to a given drive using an HMP command'''
> @@ -790,16 +799,18 @@ def dictpath(self, d, path):
>                   idx = int(idx)
>   
>               if not isinstance(d, dict) or component not in d:
> -                self.fail('failed path traversal for "%s" in "%s"' % (path, str(d)))
> +                self.fail(f'failed path traversal for "{path}" in "{d}"')
>               d = d[component]
>   
>               if m:
>                   if not isinstance(d, list):
> -                    self.fail('path component "%s" in "%s" is not a list in "%s"' % (component, path, str(d)))
> +                    self.fail(f'path component "{component}" in "{path}" '
> +                              f'is not a list in "{d}"')
>                   try:
>                       d = d[idx]
>                   except IndexError:
> -                    self.fail('invalid index "%s" in path "%s" in "%s"' % (idx, path, str(d)))
> +                    self.fail(f'invalid index "{idx}" in path "{path}" '
> +                              f'in "{d}"')
>           return d
>   
>       def assert_qmp_absent(self, d, path):
> @@ -850,10 +861,13 @@ def assert_json_filename_equal(self, json_filename, reference):
>           '''Asserts that the given filename is a json: filename and that its
>              content is equal to the given reference object'''
>           self.assertEqual(json_filename[:5], 'json:')
> -        self.assertEqual(self.vm.flatten_qmp_object(json.loads(json_filename[5:])),
> -                         self.vm.flatten_qmp_object(reference))
> +        self.assertEqual(
> +            self.vm.flatten_qmp_object(json.loads(json_filename[5:])),
> +            self.vm.flatten_qmp_object(reference)
> +        )
>   
> -    def cancel_and_wait(self, drive='drive0', force=False, resume=False, wait=60.0):
> +    def cancel_and_wait(self, drive='drive0', force=False,
> +                        resume=False, wait=60.0):
>           '''Cancel a block job and wait for it to finish, returning the event'''
>           result = self.vm.qmp('block-job-cancel', device=drive, force=force)
>           self.assert_qmp(result, 'return', {})
> @@ -877,8 +891,8 @@ def cancel_and_wait(self, drive='drive0', force=False, resume=False, wait=60.0):
>           self.assert_no_active_block_jobs()
>           return result
>   
> -    def wait_until_completed(self, drive='drive0', check_offset=True, wait=60.0,
> -                             error=None):
> +    def wait_until_completed(self, drive='drive0', check_offset=True,
> +                             wait=60.0, error=None):
>           '''Wait for a block job to finish, returning the event'''
>           while True:
>               for event in self.vm.get_qmp_events(wait=wait):
> @@ -1017,8 +1031,11 @@ def verify_quorum():
>           notrun('quorum support missing')
>   
>   def qemu_pipe(*args):
> -    '''Run qemu with an option to print something and exit (e.g. a help option),
> -    and return its output'''
> +    """
> +    Run qemu with an option to print something and exit (e.g. a help option).
> +
> +    :return: QEMU's stdout output.
> +    """
>       args = [qemu_prog] + qemu_opts + list(args)
>       subp = subprocess.Popen(args, stdout=subprocess.PIPE,
>                               stderr=subprocess.STDOUT,
> @@ -1056,8 +1073,8 @@ def func_wrapper(test_case: QMPTestCase, *args, **kwargs):
>   
>               usf_list = list(set(fmts) - set(supported_formats(read_only)))
>               if usf_list:
> -                test_case.case_skip('{}: formats {} are not whitelisted'.format(
> -                    test_case, usf_list))
> +                test_case.case_skip(f'{test_case}: formats {usf_list} '
> +                                    'are not whitelisted')
>                   return None
>               else:
>                   return func(test_case, *args, **kwargs)
> diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc
> index 8720b6a0de..8d02f00607 100644
> --- a/tests/qemu-iotests/pylintrc
> +++ b/tests/qemu-iotests/pylintrc
> @@ -19,4 +19,8 @@ disable=invalid-name,
>           too-many-public-methods,
>           # These are temporary, and should be removed:
>           missing-docstring,
> -        line-too-long,
> +
> +[FORMAT]
> +
> +# Maximum number of characters on a single line.
> +max-line-length=79
> 

On 3/4/20 4:58 PM, Philippe Mathieu-Daudé wrote:
> Do we want to indent Python like C and align argument below opening
> parenthesis? Except when using sys.stderr.write() you seem to do it.

This isn't an argument to write, it's an argument to the format string,
so I will say "no."

For *where* I've placed the line break, this is the correct indentation.
emacs's python mode will settle on this indent, too.

https://python.org/dev/peps/pep-0008/#indentation

(If anyone quotes Guido's belittling comment in this email, I will
become cross.)


But there are other places to put the line break. This is also
technically valid:

sys.stderr.write('qemu-img received signal %i: %s\n'
                 % (-exitcode, ' '.join(qemu_img_args + list(args))))

And so is this:

    sys.stderr.write('qemu-img received signal %i: %s\n' %
                     (-exitcode, ' '.join(qemu_img_args + list(args))))

(And so would be any other number of rewrites to use format codes,
f-strings, or temporary variables to otherwise reduce the length of the
line.)

I will reluctantly admit that wrapping to 79 columns is useful in some
contexts. The beauty of line continuations is something I have little
desire to litigate, though.

If there's some consensus on the true and beautiful way to do it, I will
oblige -- but the thought of spinning more iterations until we find a
color swatch we like is an unpleasant one.

--js

Am 05.03.2020 um 00:14 hat John Snow geschrieben:
> 
> 
> On 3/4/20 4:58 PM, Philippe Mathieu-Daudé wrote:

Adding back the context:

> -        sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' '.join(qemu_img_args + list(args))))
> +        sys.stderr.write('qemu-img received signal %i: %s\n' % (
> +            -exitcode, ' '.join(qemu_img_args + list(args))))

> > Do we want to indent Python like C and align argument below opening
> > parenthesis? Except when using sys.stderr.write() you seem to do it.
> 
> This isn't an argument to write, it's an argument to the format string,
> so I will say "no."

The argument to write() is an expression. This expression contains the %
operator with both of its operands. It's still fully within the
parentheses of write(), so I think Philippe's question is valid.

> For *where* I've placed the line break, this is the correct indentation.
> emacs's python mode will settle on this indent, too.
> 
> https://python.org/dev/peps/pep-0008/#indentation

The PEP-8 examples are not nested, so it's not completely clear. I
wonder if hanging indents wouldn't actually mean the following because
if you line wrap an argument list (which contains the whole %
expression), you're supposed to have nothing else on the line of the
opening parenthesis:

    sys.stderr.write(
        'qemu-img received signal %i: %s\n'
        % (-exitcode, ' '.join(qemu_img_args + list(args))))

But anyway, I think the question is more whether we want to use hanging
indents at all (or at least if we want to use it even in cases where the
opening parenthesis isn't already at like 70 characters) when we're
avoiding it in our C coding style.

There's no technical answer to this, it's a question of our preferences.

> (If anyone quotes Guido's belittling comment in this email, I will
> become cross.)
> 
> 
> But there are other places to put the line break. This is also
> technically valid:
> 
> sys.stderr.write('qemu-img received signal %i: %s\n'
>                  % (-exitcode, ' '.join(qemu_img_args + list(args))))
> 
> And so is this:
> 
>     sys.stderr.write('qemu-img received signal %i: %s\n' %
>                      (-exitcode, ' '.join(qemu_img_args + list(args))))

PEP-8 suggests the former, but allows both styles:

https://www.python.org/dev/peps/pep-0008/#should-a-line-break-before-or-after-a-binary-operator

> (And so would be any other number of rewrites to use format codes,
> f-strings, or temporary variables to otherwise reduce the length of the
> line.)
> 
> I will reluctantly admit that wrapping to 79 columns is useful in some
> contexts. The beauty of line continuations is something I have little
> desire to litigate, though.
> 
> If there's some consensus on the true and beautiful way to do it, I will
> oblige -- but the thought of spinning more iterations until we find a
> color swatch we like is an unpleasant one.

I'll accept any colour for the bikeshed, as long as it's green. ;-)

Kevin

On 3/5/20 6:55 AM, Kevin Wolf wrote:
> Am 05.03.2020 um 00:14 hat John Snow geschrieben:
>>
>>
>> On 3/4/20 4:58 PM, Philippe Mathieu-Daudé wrote:
> 
> Adding back the context:
> 
>> -        sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' '.join(qemu_img_args + list(args))))
>> +        sys.stderr.write('qemu-img received signal %i: %s\n' % (
>> +            -exitcode, ' '.join(qemu_img_args + list(args))))
> 
>>> Do we want to indent Python like C and align argument below opening
>>> parenthesis? Except when using sys.stderr.write() you seem to do it.
>>
>> This isn't an argument to write, it's an argument to the format string,
>> so I will say "no."
> 
> The argument to write() is an expression. This expression contains the %
> operator with both of its operands. It's still fully within the
> parentheses of write(), so I think Philippe's question is valid.
> 
>> For *where* I've placed the line break, this is the correct indentation.
>> emacs's python mode will settle on this indent, too.
>>
>> https://python.org/dev/peps/pep-0008/#indentation
> 
> The PEP-8 examples are not nested, so it's not completely clear. I
> wonder if hanging indents wouldn't actually mean the following because
> if you line wrap an argument list (which contains the whole %
> expression), you're supposed to have nothing else on the line of the
> opening parenthesis:
> 
>     sys.stderr.write(
>         'qemu-img received signal %i: %s\n'
>         % (-exitcode, ' '.join(qemu_img_args + list(args))))
> 

This is fine too.

> But anyway, I think the question is more whether we want to use hanging
> indents at all (or at least if we want to use it even in cases where the
> opening parenthesis isn't already at like 70 characters) when we're
> avoiding it in our C coding style.
> 
> There's no technical answer to this, it's a question of our preferences.
> 

Maybe it is ambiguous. Long lines are just ugly everywhere.

>> (If anyone quotes Guido's belittling comment in this email, I will
>> become cross.)
>>
>>
>> But there are other places to put the line break. This is also
>> technically valid:
>>
>> sys.stderr.write('qemu-img received signal %i: %s\n'
>>                  % (-exitcode, ' '.join(qemu_img_args + list(args))))
>>
>> And so is this:
>>
>>     sys.stderr.write('qemu-img received signal %i: %s\n' %
>>                      (-exitcode, ' '.join(qemu_img_args + list(args))))
> 
> PEP-8 suggests the former, but allows both styles:
> 
> https://www.python.org/dev/peps/pep-0008/#should-a-line-break-before-or-after-a-binary-operator
> 

So in summary:

- Avoid nested hanging indents from format operators
- Use a line break before the % format operator.
- OPTIONALLY(?), use a hanging indent for the entire format string to
reduce nesting depth.


e.g., either this form:
(using a line break before the binary operator and nesting to the
argument level)

write('hello %s'
      % (world,))


or optionally this form if it buys you a little more room:
(using a hanging indent of 4 spaces and nesting arguments at that level)

write(
    'hello %s'
    % ('world',))


but not ever this form:
(Using a hanging indent of 4 spaces from the opening paren of the format
operand)

write('hello %s' % (
    'world',))



yea/nea?

(Kevin, Philippe, Markus, Max)

>> (And so would be any other number of rewrites to use format codes,
>> f-strings, or temporary variables to otherwise reduce the length of the
>> line.)
>>
>> I will reluctantly admit that wrapping to 79 columns is useful in some
>> contexts. The beauty of line continuations is something I have little
>> desire to litigate, though.
>>
>> If there's some consensus on the true and beautiful way to do it, I will
>> oblige -- but the thought of spinning more iterations until we find a
>> color swatch we like is an unpleasant one.
> 
> I'll accept any colour for the bikeshed, as long as it's green. ;-)
> 
> Kevin
> 

Am 05.03.2020 um 19:25 hat John Snow geschrieben:
> On 3/5/20 6:55 AM, Kevin Wolf wrote:
> > Am 05.03.2020 um 00:14 hat John Snow geschrieben:
> >>
> >>
> >> On 3/4/20 4:58 PM, Philippe Mathieu-Daudé wrote:
> > 
> > Adding back the context:
> > 
> >> -        sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' '.join(qemu_img_args + list(args))))
> >> +        sys.stderr.write('qemu-img received signal %i: %s\n' % (
> >> +            -exitcode, ' '.join(qemu_img_args + list(args))))
> > 
> >>> Do we want to indent Python like C and align argument below opening
> >>> parenthesis? Except when using sys.stderr.write() you seem to do it.
> >>
> >> This isn't an argument to write, it's an argument to the format string,
> >> so I will say "no."
> > 
> > The argument to write() is an expression. This expression contains the %
> > operator with both of its operands. It's still fully within the
> > parentheses of write(), so I think Philippe's question is valid.
> > 
> >> For *where* I've placed the line break, this is the correct indentation.
> >> emacs's python mode will settle on this indent, too.
> >>
> >> https://python.org/dev/peps/pep-0008/#indentation
> > 
> > The PEP-8 examples are not nested, so it's not completely clear. I
> > wonder if hanging indents wouldn't actually mean the following because
> > if you line wrap an argument list (which contains the whole %
> > expression), you're supposed to have nothing else on the line of the
> > opening parenthesis:
> > 
> >     sys.stderr.write(
> >         'qemu-img received signal %i: %s\n'
> >         % (-exitcode, ' '.join(qemu_img_args + list(args))))
> > 
> 
> This is fine too.
> 
> > But anyway, I think the question is more whether we want to use hanging
> > indents at all (or at least if we want to use it even in cases where the
> > opening parenthesis isn't already at like 70 characters) when we're
> > avoiding it in our C coding style.
> > 
> > There's no technical answer to this, it's a question of our preferences.
> > 
> 
> Maybe it is ambiguous. Long lines are just ugly everywhere.
> 
> >> (If anyone quotes Guido's belittling comment in this email, I will
> >> become cross.)
> >>
> >>
> >> But there are other places to put the line break. This is also
> >> technically valid:
> >>
> >> sys.stderr.write('qemu-img received signal %i: %s\n'
> >>                  % (-exitcode, ' '.join(qemu_img_args + list(args))))
> >>
> >> And so is this:
> >>
> >>     sys.stderr.write('qemu-img received signal %i: %s\n' %
> >>                      (-exitcode, ' '.join(qemu_img_args + list(args))))
> > 
> > PEP-8 suggests the former, but allows both styles:
> > 
> > https://www.python.org/dev/peps/pep-0008/#should-a-line-break-before-or-after-a-binary-operator
> > 
> 
> So in summary:
> 
> - Avoid nested hanging indents from format operators
> - Use a line break before the % format operator.
> - OPTIONALLY(?), use a hanging indent for the entire format string to
> reduce nesting depth.

Yes, though I don't think of it as a special case for format strings. So
I would phrase it like this:

- Don't use hanging indent for any nested parentheses unless the outer
  parentheses use hanging indents, too.
- Use a line break before binary operators.
- OPTIONALLY, use a hanging indent for the top level(s) to reduce
  nesting depth.

The first one is the only rule that involves some interpretation of
PEP-8, the rest seems to be its unambiguous recommendation.

Anyway, so I would apply the exact same rules to the following (imagine
even longer expressions, especially the last example doesn't make sense
with the short numbers):

* bad:
    really_long_function_name(-1234567890 + 987654321 * (
        1337 / 42))

* ok:
    really_long_function_name(-1234567890 + 987654321
                              * (1337 / 42))

* ok:
    really_long_function_name(
        -1234567890 + 987654321
        * (1337 / 42))

* ok:
    really_long_function_name(
        -1234567890 + 987654321 * (
            1337 / 42))

> e.g., either this form:
> (using a line break before the binary operator and nesting to the
> argument level)
> 
> write('hello %s'
>       % (world,))
> 
> 
> or optionally this form if it buys you a little more room:
> (using a hanging indent of 4 spaces and nesting arguments at that level)
> 
> write(
>     'hello %s'
>     % ('world',))
> 
> 
> but not ever this form:
> (Using a hanging indent of 4 spaces from the opening paren of the format
> operand)
> 
> write('hello %s' % (
>     'world',))
> 
> 
> 
> yea/nea?
> 
> (Kevin, Philippe, Markus, Max)

Looks good to me.

Kevin

Kevin Wolf <kwolf@redhat.com> writes:

> Am 05.03.2020 um 19:25 hat John Snow geschrieben:
[...]
>> So in summary:
>> 
>> - Avoid nested hanging indents from format operators
>> - Use a line break before the % format operator.
>> - OPTIONALLY(?), use a hanging indent for the entire format string to
>> reduce nesting depth.
>
> Yes, though I don't think of it as a special case for format strings. So
> I would phrase it like this:
>
> - Don't use hanging indent for any nested parentheses unless the outer
>   parentheses use hanging indents, too.
> - Use a line break before binary operators.
> - OPTIONALLY, use a hanging indent for the top level(s) to reduce
>   nesting depth.
>
> The first one is the only rule that involves some interpretation of
> PEP-8, the rest seems to be its unambiguous recommendation.
>
> Anyway, so I would apply the exact same rules to the following (imagine
> even longer expressions, especially the last example doesn't make sense
> with the short numbers):
>
> * bad:
>     really_long_function_name(-1234567890 + 987654321 * (
>         1337 / 42))

Definitely bad.

> * ok:
>     really_long_function_name(-1234567890 + 987654321
>                               * (1337 / 42))
>
> * ok:
>     really_long_function_name(
>         -1234567890 + 987654321
>         * (1337 / 42))

Yup.

> * ok:
>     really_long_function_name(
>         -1234567890 + 987654321 * (
>             1337 / 42))

Okay, although when you need this, chances are there's just too much
going on in that argument list.

>> e.g., either this form:
>> (using a line break before the binary operator and nesting to the
>> argument level)
>> 
>> write('hello %s'
>>       % (world,))
>> 
>> 
>> or optionally this form if it buys you a little more room:
>> (using a hanging indent of 4 spaces and nesting arguments at that level)
>> 
>> write(
>>     'hello %s'
>>     % ('world',))
>> 
>> 
>> but not ever this form:
>> (Using a hanging indent of 4 spaces from the opening paren of the format
>> operand)
>> 
>> write('hello %s' % (
>>     'world',))
>> 
>> 
>> 
>> yea/nea?
>> 
>> (Kevin, Philippe, Markus, Max)
>
> Looks good to me.

Me too.

On 3/6/20 5:14 AM, Kevin Wolf wrote:
> Am 05.03.2020 um 19:25 hat John Snow geschrieben:
>> On 3/5/20 6:55 AM, Kevin Wolf wrote:
>>> Am 05.03.2020 um 00:14 hat John Snow geschrieben:
>>>>
>>>>
>>>> On 3/4/20 4:58 PM, Philippe Mathieu-Daudé wrote:
>>>
>>> Adding back the context:
>>>
>>>> -        sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' '.join(qemu_img_args + list(args))))
>>>> +        sys.stderr.write('qemu-img received signal %i: %s\n' % (
>>>> +            -exitcode, ' '.join(qemu_img_args + list(args))))
>>>
>>>>> Do we want to indent Python like C and align argument below opening
>>>>> parenthesis? Except when using sys.stderr.write() you seem to do it.
>>>>
>>>> This isn't an argument to write, it's an argument to the format string,
>>>> so I will say "no."
>>>
>>> The argument to write() is an expression. This expression contains the %
>>> operator with both of its operands. It's still fully within the
>>> parentheses of write(), so I think Philippe's question is valid.
>>>
>>>> For *where* I've placed the line break, this is the correct indentation.
>>>> emacs's python mode will settle on this indent, too.
>>>>
>>>> https://python.org/dev/peps/pep-0008/#indentation
>>>
>>> The PEP-8 examples are not nested, so it's not completely clear. I
>>> wonder if hanging indents wouldn't actually mean the following because
>>> if you line wrap an argument list (which contains the whole %
>>> expression), you're supposed to have nothing else on the line of the
>>> opening parenthesis:
>>>
>>>     sys.stderr.write(
>>>         'qemu-img received signal %i: %s\n'
>>>         % (-exitcode, ' '.join(qemu_img_args + list(args))))
>>>
>>
>> This is fine too.
>>
>>> But anyway, I think the question is more whether we want to use hanging
>>> indents at all (or at least if we want to use it even in cases where the
>>> opening parenthesis isn't already at like 70 characters) when we're
>>> avoiding it in our C coding style.
>>>
>>> There's no technical answer to this, it's a question of our preferences.
>>>
>>
>> Maybe it is ambiguous. Long lines are just ugly everywhere.
>>
>>>> (If anyone quotes Guido's belittling comment in this email, I will
>>>> become cross.)
>>>>
>>>>
>>>> But there are other places to put the line break. This is also
>>>> technically valid:
>>>>
>>>> sys.stderr.write('qemu-img received signal %i: %s\n'
>>>>                  % (-exitcode, ' '.join(qemu_img_args + list(args))))
>>>>
>>>> And so is this:
>>>>
>>>>     sys.stderr.write('qemu-img received signal %i: %s\n' %
>>>>                      (-exitcode, ' '.join(qemu_img_args + list(args))))
>>>
>>> PEP-8 suggests the former, but allows both styles:
>>>
>>> https://www.python.org/dev/peps/pep-0008/#should-a-line-break-before-or-after-a-binary-operator
>>>
>>
>> So in summary:
>>
>> - Avoid nested hanging indents from format operators
>> - Use a line break before the % format operator.
>> - OPTIONALLY(?), use a hanging indent for the entire format string to
>> reduce nesting depth.
> 
> Yes, though I don't think of it as a special case for format strings. So
> I would phrase it like this:
> 
> - Don't use hanging indent for any nested parentheses unless the outer
>   parentheses use hanging indents, too.
> - Use a line break before binary operators.
> - OPTIONALLY, use a hanging indent for the top level(s) to reduce
>   nesting depth.
> 
> The first one is the only rule that involves some interpretation of
> PEP-8, the rest seems to be its unambiguous recommendation.
> 
> Anyway, so I would apply the exact same rules to the following (imagine
> even longer expressions, especially the last example doesn't make sense
> with the short numbers):
> 
> * bad:
>     really_long_function_name(-1234567890 + 987654321 * (
>         1337 / 42))
> 
> * ok:
>     really_long_function_name(-1234567890 + 987654321
>                               * (1337 / 42))
> 
> * ok:
>     really_long_function_name(
>         -1234567890 + 987654321
>         * (1337 / 42))
> 
> * ok:
>     really_long_function_name(
>         -1234567890 + 987654321 * (
>             1337 / 42))
> 
>> e.g., either this form:
>> (using a line break before the binary operator and nesting to the
>> argument level)
>>
>> write('hello %s'
>>       % (world,))
>>
>>
>> or optionally this form if it buys you a little more room:
>> (using a hanging indent of 4 spaces and nesting arguments at that level)
>>
>> write(
>>     'hello %s'
>>     % ('world',))
>>
>>
>> but not ever this form:
>> (Using a hanging indent of 4 spaces from the opening paren of the format
>> operand)
>>
>> write('hello %s' % (
>>     'world',))
>>
>>
>>
>> yea/nea?
>>
>> (Kevin, Philippe, Markus, Max)
> 
> Looks good to me.
> 
> Kevin
> 

Great, thanks!

I am sorry for having been so tetchy. I appreciate the reviews.

--js