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

Fix/requirements #843

Closed
wants to merge 4 commits into from
Closed

Fix/requirements #843

wants to merge 4 commits into from

Conversation

drodarie
Copy link
Contributor

@drodarie drodarie commented May 9, 2024

  • Refactor PackageRequirement to extend from types.object_
  • Change ConfigurationListAttribute and ConfigurationDictAttribute

closes #842

@drodarie drodarie requested a review from Helveg May 9, 2024 10:03
@drodarie drodarie self-assigned this May 9, 2024
@drodarie
Copy link
Contributor Author

drodarie commented May 9, 2024

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?

@@ -418,6 +418,9 @@ def __init__(self, module=None, **kwargs):
if module is not None:
self.module = module

def __inv__(self):
return self.module
Copy link
Contributor

@Helveg Helveg May 11, 2024

Choose a reason for hiding this comment

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

What about self.attr? is it unused in all this?

@Helveg
Copy link
Contributor

Helveg commented May 11, 2024

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

Helveg commented May 11, 2024

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

@Helveg Helveg closed this May 11, 2024
@drodarie drodarie reopened this May 11, 2024
@drodarie
Copy link
Contributor Author

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

Helveg commented May 11, 2024

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

drodarie commented May 12, 2024

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.

@drodarie drodarie closed this May 13, 2024
@Helveg
Copy link
Contributor

Helveg commented May 14, 2024

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__.

@drodarie drodarie mentioned this pull request May 14, 2024
2 tasks
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.

Configuration Packages are not JSON serializable
2 participants