Guideline for attributes

[TeresiaOlsson]

When going through the code I found different ways of how attributes are implemented and I also wasn't entirely sure what to go for myself when doing changes. So I thought deciding on a guideline would be good.

This is my suggestion:

• For attributes that are meant to be public to users or other classes and where no special logic is needed for getting/settings, use regular attributes, e.g. self.x.

• For attributes that are intended to be private to a class and never called by another class use single underscore, e.g. self._x.'
  If another class calls such an attribute, it means the implementation is wrong and should be modified.

• For attributes that need special logic for getting/setting, use single underscore + property decorator for getter/setter. Only make functions like get_something and set_something if there is a specific use case for it.

• Never use double underscore unless you have a specific use case where it is necessary. This is because name mangling make the code more difficult to understand and debug for non-expert Python programmers.

[gubaidulinvadim]

I think your suggestion is alright. I only slightly disagree on get and set. I don't mind so much having get/set functions instead of properties, even if it is not so Pythonic. Maybe the guidelines should be added to CONTRIBUTING.md or to some other visible place.

[TeresiaOlsson]

I think get/set functions are okay if there is a valid reason for it but since it's not the Pythonic way it's not a good idea to overuse it. Better for the future to embrace the philosophy of the chosen language since it makes the code easier to understand for new users and developers.

[TeresiaOlsson]

I also discovered now that using properties makes it easier to implement a generic __repr__ function which automatically prints all the attributes that are meant to be public for the users. So far we have relied on there being a ConfigModel where you can get those attributes for the __repr__ but if we decide to not enforce config models for all classes to make things more flexible we need some other option to automatically generate the __repr__ or we have to implement it on each class which is annoying.

[TeresiaOlsson]

@JeanLucPons What do you think about this?

[JeanLucPons]

I'm not sure to fully understand, do you want to add some _ on configuration fields that are not supposed to be changed by the user ?
If I get back my previous example:

# Set up the  chromaticy monitor (override config settings)
CM = SR.get_chromaticity_monitor("CHROMATICITY_MONITOR")
CM.n_step =3      # 3 point for chroma fit
CM.n_avg_meas = 1  # No averaging`

And in the model you _ all fields that are supposed to be read only (static config) for instance ?

[TeresiaOlsson]

The suggestion is just to follow the Python standard that private attributes that are not meant to be part of the public interface of a class should use a _. This is to indicate to the developers that they should not be called by users or other classes, but only internally in the same class.

The attributes that are part of the public interface should have no underscore. For cases where the attribute should be read only or you need some logic in the setter (for example to validate the data before changing it) you would make a private attribute and combine it with a property for implementing the getter/setter functionality.

For your example I imagine that n_step and n_avg_meas probably can be normal public attributes. If there also is a name attribute however that needs to be private and with properties for the logic because changing to a non unique name shouldn't be allowed. For example maybe something like this:

class ChromaticityMonitor:
    def __init__(self, name: str, n_step: int, n_avg_meas: int):
        self.name = name          # Uses the setter
        self.n_step = n_step
        self.n_avg_meas = n_avg_meas

    @property
    def name(self) -> str:
        return self._name

    @name.setter
    def name(self, value: str) -> None:
        # some logic to ensure that the new name is unique before setting it
        self._name = value

Personally, I however think the users should never be allowed to change the configuration like this because validation then needs to be implemented on per-class basis and the same class is responsible for both storing the configuration and performing the business logic which is not good separation of concerns.

I rather think we should follow the MML approach and have one central ConfigurationRegistry which is responsible for being the single source of truth for all configuration data. The ChromaticityMonitor will then only be responsible for the business logic. It will be built by extracting the required configuration from the registry and when the user wants to change it, they will do the change in the registry and the registry is responsible for updating the data where needed.