Re: Opinions on two pylint warnings?

[Bruno Haible]

Hi Collin,

> The first is W0707 [1]. It recommends chaining exceptions. So this:
> 
>     try:  # Try to apply patch
>         sp.check_call(command, shell=True)
>     except sp.CalledProcessError:
>         raise GLError(2, name)
> 
> becomes:
> 
>     try:  # Try to apply patch
>         sp.check_call(command, shell=True)
>     except sp.CalledProcessError as exc:
>         raise GLError(2, name) from exc
> 
> This is used for the exception backtrace messages.

Let's see what the effect is:

===================== foo.py ==================
import subprocess as sp

class GLError(Exception):
   '''foo'''
   def __init__(self, errno: int) -> None:
       pass

try:
    sp.check_call('boom', shell=True)
except sp.CalledProcessError as x:
    raise GLError(2) from x
===============================================

This produces the output:

--------------------------------------------------------------------------------
/bin/sh: 1: boom: not found
Traceback (most recent call last):
  File "/home/bruno/foo.py", line 9, in <module>
    sp.check_call('boom', shell=True)
  File "/usr/lib/python3.10/subprocess.py", line 369, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command 'boom' returned non-zero exit status 127.

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/bruno/foo.py", line 11, in <module>
    raise GLError(2) from x
__main__.GLError: 2
--------------------------------------------------------------------------------

And without the chaining, it is:

--------------------------------------------------------------------------------
/bin/sh: 1: boom: not found
Traceback (most recent call last):
  File "/home/bruno/foo.py", line 9, in <module>
    sp.check_call('boom', shell=True)
  File "/usr/lib/python3.10/subprocess.py", line 369, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command 'boom' returned non-zero exit status 127.

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/bruno/foo.py", line 11, in <module>
    raise GLError(2)
__main__.GLError: 2
--------------------------------------------------------------------------------

The only difference between the two outputs is the message between the two
stack traces. The latter sounds like a program bug; therefore I would say,
let's use the chaining — just in order to clarify that the second exception
is intended.

> The second warning is R1735 [1]. It suggests changing 'dict()' to
> '{}'. I am fine with making that change or silencing the warning.
> 
> I prefer the appearance of '{}', but can understand why some might
> dislike it:
> 
>    Dictionary:
>        var = {} # Empty dictionary.
>        # {0: 'a', 1: 'b', 2: 'c'}
>        var = {i : chr(ord('a') + i) for i in range(3)} 
> 
>    Set:
>        var = set() # We are stuck with this. :(
>        # Might be mistaken for a dictionary at a quick glance?
>        # {1, 2, 3}
>        var = {value for value in [1, 1, 2, 2, 3]}

Oh, indeed there's a fallacy here: {} denotes a dict, not a set!

>>> type({'x','y'})
<class 'set'>
>>> type({'x'})
<class 'set'>
>>> type({})
<class 'dict'>

This is surprising enough that we should add to our coding style:

  - Never use the {} literal, because it denotes a dictionary,
    as opposed to {x}, {x,y}, etc., which denote sets.

Bruno