pasqal-io / Pulser

Library for pulse-level/analog control of neutral atom devices. Emulator with QuTiP.
Apache License 2.0
171 stars 60 forks source link

Select a coding style for instance attributes definition #335

Open Codoscope opened 2 years ago

Codoscope commented 2 years ago

We had a discussion with @HGSilveri about where instance attributes should be type annotated. I dug a bit more on the subject, and here is what I can come with:

  1. declare attributes at class scope:
    class A:
       x: int # pro: all instance attributes grouped here,
              # making it easy to see them all at a glance
       def __init__(self): self.x = 0 # cons: self.x = 0 could be absent and mypy wouldn't complain

    the cons is often circumvented by UTs, and it would be fixed with https://github.com/python/mypy/issues/4019

  2. declare attributes at the top of __init__ (what is done here): same pro in a weaker form, and identical cons
  3. keep attributes sparsed inside __init__:
    class A:
       # cons: need to read the whole class to find all instance attributes
       def __init__(self): self.x: int = 0 # pro: x declared and defined at once, helping not to forget to define it
  4. attributes could be both declared and defined at the class scope, which would be correct for static type checking, but would also create class attributes:

    class A:
       x: int = 0 # pros: all instance attributes grouped here, and it's clear that x is defined (only works for defaults)
    A.x # cons: this works
    vars(A()) # cons: this is empty
    
    # big cons: default value is a class attribute, until a new value is provided,
    # so all instances share it by default until they are given their own
    class B:
       x: list = []
    b1 = B()
    b2 = B()
    b1.x.append(1)
    b2.x # also contains 1
    b2.x = [] # b2 has finally its own instance attribute

    for more details, see:

The options 1 and 3 are probably the best ones between which we should choose (I would personnally go for the 1).

In all cases, I think we should avoid instance attribute declarations that are neither at the classe scope, neither inside __init__, but inside another method, because this basically cumulates cons of 1 and 3, and doesn't have any of the mentioned pros.

lvignoli commented 2 years ago

It’s easy to prevent 4.: just never ever use a mutable default value

class C:
    x: list = None  # do
    y: list = []  # don't

a, b = C(), C()
a.x.append(1)  # fails, since it is type None
a.x = [1]
print(a.x, "and", b.x)
## [1] and None
# otherwise same problem for a.y, b.y
lvignoli commented 2 years ago

While 1. Is the most readable and pleasing to me—and I would really like to be able to do so blindly—I think 3. is the safest and simplest choice. Otherwise there is a great confusion between the instances and class attributes, and we are dangerously close to case 4 with bad bugs. Moreover if you have many properties defined on your class, the whole point of grouping attributes makes little sense.

The (public) instance attributes should be listed in the class docstring with their types, à la Google. I think most IDEs do a good job at discovering objects attributes. Together with the class docstring this should be enough.

HGSilveri commented 2 years ago

I'm with @lvignoli - I like the cleanliness of option 1., but I don't think it's worth the potential for confusion. I would go with 3. too.

My only qualm with making a style decision of this type is enforcing it. Would this come with a checker for CI? If not, I find it very hard as a reviewer to remember to check for this.