PyEllips / pyElli

An open source ellipsometry analysis tool for reproducible and comprehensible building of optical models.
https://pyelli.readthedocs.io
GNU General Public License v3.0
17 stars 6 forks source link

Move class attributes to variables #187

Closed MarJMue closed 3 days ago

MarJMue commented 3 weeks ago

Remove all class attributes, because they could be used to change values for all objects.

MarJMue commented 3 days ago

I completely agree, that we could use the private notation or switch to using properties.

Class variables are shared across all instances, unless they get overwritten on instance level. This makes it possible to set something like MyClass.var = "foo" and change this for every instance. I just recently discovered this feature and that dataclasses are defined in an unusual way, which I thought was the norm.

The thought was to avoid some of the possible mistakes altogether, but I suppose it might be a bit too paranoid :wink:
On second glance, it should only be possible for material rotation.

domna commented 3 days ago

I think it's a good idea to try to prevent these mistakes but I'm afraid it's not really possible with python. In some cases this also produces non-recommended python code, i.e., when the variable is just set in a sub-function and not in the class or the __init__ directly. Of course it still works with python but it's really hard to understand when reading the code (because we're so used to get all these variables listed in the class or it's constructor - that is why I actually like listing the fields in the class directly).

How shall we proceed with this PR? Keep the fields listing in the class but make the fields private by adding an underscore in front?

MarJMue commented 3 days ago

I think best would be to define all variables in the __init__ method, but the problem is so small, we can leave it as is for now.