dbbs-lab / bsb-core

The Brain Scaffold Builder
https://bsb.readthedocs.io
GNU General Public License v3.0
22 stars 16 forks source link

Fix/requirements #843

Closed drodarie closed 6 months ago

drodarie commented 6 months ago

closes #842

drodarie commented 6 months ago

For some reason ConfigurationListAttribute and ConfigurationDictAttribute type was returning the fill function. But type is also called in a condition of ConfigurationAttribute.tree_of to obtain the __inv__ attribute.

@Helveg, I am not sure why this was implemented like this?

Helveg commented 6 months ago

For some reason ConfigurationListAttribute and ConfigurationDictAttribute type was returning the fill function. But type is also called in a condition of ConfigurationAttribute.tree_of to obtain the inv attribute.

This is intentional. The type function is used to convert the raw values to the parsed values. A config list has type int, but must parse [1,2,3], which is not int, but list, so the fill function is generated to parse lists of ints. Look through the source code though, there is already a property stored somewhere that contains the element type, IIRC.

Edit: It's child_type.

Helveg commented 6 months ago

Closing this because there must remain a difference between the final type function, which converts the node's given value to its final value, and specialized container nodes such as lists' and dicts' child_type. This problem must be solved another way.

I think the correct way is for the fill function to also expose its child_types __inv__.

Feel free to reopen with new proposal

drodarie commented 6 months ago

Reopening just to get some precision. Will make a new pull request once I have re-implemented it correctly.

This is intentional. The type function is used to convert the raw values to the parsed values. A config list has type int, but must parse [1,2,3], which is not int, but list, so the fill function is generated to parse lists of ints. Look through the source code though, there is already a property stored somewhere that contains the element type, IIRC.

Edit: It's child_type.

I am not sure I understand. If type is a function like in ConfigurationListAttribute._set_type:

def _set_type(self, type, key=None):
        self.child_type = super()._set_type(type, key=False)
        return self.fill

then why in tree_of is it not treated like so?

if hasattr(self.type, "__inv__") and value is not None:
    value = self.type.__inv__(value)
Helveg commented 6 months ago

In Python functions are objects too, of the wider family of "callable objects", and have many attributes. In this snippet we check for an __inv__ attribute, a perfectly valid operation to execute on a function object:

if hasattr(self.type, "__inv__") and value is not None:
    value = self.type.__inv__(value)

Look at the TypeHandler class, most type handlers that have an __inv__ attribute will have it because they're instances of classes that have inherited from TypeHandler and have an __inv__ method in their class.

But anyone could create any object they want and add __call__ and __inv__, or define any function they want and then assign it manually: f.__inv__ = some_other_f

drodarie commented 6 months ago

Ok let me see if I understand correctly what the function tree_of is doing:

    def tree_of(self, value):
        # if value is a node it should have a __tree__ function
        if hasattr(value, "__tree__"):  
            value = value.__tree__()
        # if self.type is a bsb class with an  __inv__ function
        # BTW, maybe it should be elif here
        if hasattr(self.type, "__inv__") and value is not None: 
            value = self.type.__inv__(value)
        # else return value as it is
        return value

So we need to deal with the cases where value is not a Node and self.type does not have a __inv__. it is quite difficult to predict what should be self.type, so I will do as you suggested and expose an attribute __inv__ for the fill function if its children have it.

Helveg commented 6 months ago

Your analysis of the tree_of function seems incorrect: this is a function of a descriptor, self is the class of the attribute e.g. ConfigurationListAttribute.

# if value is a node it should have a __tree__ function

value, can have a __tree__ function (n.b., it doesn't have to be a node per se, other things can give themselves such an attribute too).

# if self.type is a bsb class with an __inv__ function

What's a bsb class? 😋 self being the descriptor, has a type function, meaning: "when I, the descriptor, am put on another class, any value that gets set on that attribute of that class, will be converted using this type function, thereby describing that class' attribute":

class X:
  a = config.attr(...)
  b = ConfigurationAttribute(...)

class Y:
  a = config.attr(...)

The self in tree_of are instances of ConfigurationAttribute, for example: X.a will be ConfigurationAttribute, but X().a will be the result of X.a.__get__(X()), this is the Python descriptor protocol. https://docs.python.org/3/howto/descriptor.html

End of tangent: no "bsb class" has an __inv__ function, at this point we are checking whether the descriptor has an __inv__ function (for example, whether CodeDependencyNode.module has __inv__ logic or not).

# BTW, maybe it should be elif here

It is valid for __tree__ to return a value that still needs to be inverted as well (I do not think it occurs at the moment, but I don't see why we should exclude it).

I would say tree_of is working entirely as expected, and the most logical solution is to make sure that _set_type sets a type function for config dicts and lists that when it hits tree_of have an __inv__.