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

Move class attributes to variables #187

Closed
wants to merge 1 commit into from
Closed

Conversation

MarJMue
Copy link
Collaborator

@MarJMue MarJMue commented Aug 19, 2024

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

@MarJMue MarJMue marked this pull request as ready for review August 19, 2024 09:54
@MarJMue MarJMue requested a review from domna August 22, 2024 08:56
Copy link
Member

@domna domna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't fully understand these changes. This just removes the variables from the class overview but they are still set in the __init__ methods (or submethods of it). This also does not prevent any modification as python allows setting any variable name to an object during runtime.

If we want to denote that this variables are not to be messed with we should declare them private by convention, e.g., using _ in front.

@MarJMue
Copy link
Collaborator Author

MarJMue commented Sep 9, 2024

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 😉
On second glance, it should only be possible for material rotation.

@domna
Copy link
Member

domna commented Sep 9, 2024

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

MarJMue commented Sep 9, 2024

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.

@MarJMue MarJMue closed this Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants