Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Select a coding style for instance attributes definition #335

Open
Codoscope opened this issue Feb 23, 2022 · 3 comments
Open

Select a coding style for instance attributes definition #335

Codoscope opened this issue Feb 23, 2022 · 3 comments
Labels
archive Smthg to keep in mind, but not worth implementing now style Style and format related issues

Comments

@Codoscope
Copy link
Contributor

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 Proposal: use binder (or similar) to track which attributes are defined. python/mypy#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
Copy link
Collaborator

lvignoli commented Feb 23, 2022

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
Copy link
Collaborator

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
Copy link
Collaborator

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.

@HGSilveri HGSilveri added the style Style and format related issues label Feb 23, 2022
@HGSilveri HGSilveri added the archive Smthg to keep in mind, but not worth implementing now label Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
archive Smthg to keep in mind, but not worth implementing now style Style and format related issues
Projects
None yet
Development

No branches or pull requests

3 participants