`root` not supported as name of hidden attribute

[ATriantafyllopoulos]

Due to these lines which were recently added and I don't quite understand it is now not possible to do the following:

class Foo(audobject.Object):
@audobject.init_decorator(hide=['root'])
def __init__(self, root: str = None):
     self.root = root

bar = Foo('./test')
print(bar.root)  # shows './test'
bar = audobject.from_yaml_s(bar.to_yaml_s(), root='./test')
print(bar.root)  # shows None

This means that root can no longer be a hidden argument that the user would not store as YAML but define during loading. As it's kind of a common name, maybe it's not ideal to use it like this in audobject? Anyways, I guess you guys are aware of it but it gave me some trouble recently..Maybe you want to add a warning in the docs?

[hagenw]

To me this looks like a bug and not desired behavior. So, thanks for reporting.

[frankenjoe]

Yes, we introduced root to support referencing files. And the same applies also to d. It's the same issue we have in audinterface, where the user can also pass keyword arguments to the processing function but none of the arguments must have the name of a positional argument of the Process and the other interface classes.

But yeah, we should probably change it since root and also d are names a user may choose. The good news is from_dict() is usually only called under the hood by from_yaml() and also d and root are positional arguments. I.e. it's very unlikely we break code if we rename the two arguments. So we should choose some names that are not likely to be used by the user. Maybe:

__d__ and __root__?

@ATriantafyllopoulos @hagenw opinions?

[hagenw]

I don't get it completely. Are __d__ and __root__ arguments a developer has to use, or are it just internal attributes? If they are just internal attrobutes, then I think the naming __d__ and __root__ would be good.

[frankenjoe]

Are d and root arguments a developer has to use

In principle a user can do:

d = o.to_dict()
o2 = audobject.from_dict(d)

And I'm also not in favor of hiding this function as it gives the user the option to serialize to another format, e.g. json.

If they are just internal attrobutes, then I think the naming d and root would be good.

they are arguments not attributes

[hagenw]

I found it in the documentation for from_dict:

I don't see why the d argument should be a problem as it is not handed to the other functions like from_yaml_s(). I guess the problematic part is that root is further processed inside utils.get_object() and set to the object.

[frankenjoe]

don't see why the d argument should be a problem as it is not handed to the other functions like from_yaml_s()

It's a problem in the moment when the user serializes a class that a hidden argument d and then tries to overwrite the default value using kwargs:

class Foo(audobject.Object):
    @audobject.init_decorator(hide=['d'])
    def __init__(self, d: str = None):
         self.d = d

bar = Foo()
audobject.from_yaml_s(
    bar.to_yaml_s(),
    d='foo',
)

TypeError: from_dict() got multiple values for argument 'd'

[ATriantafyllopoulos]

So basically you would rename the d and root arguments inside from_dict to __d__ and __root__? I think that's a good solution. Additionally, I would check if the users has supplied those in kwargs and raise a warning. I doubt that this will ever happen but in case it does, it would help the user locate the problem

[frankenjoe]

So basically you would rename the d and root arguments inside from_dict to __d__ and __root__?

Yes!

I doubt that this will ever happen but in case it does, it would help the user locate the problem

Well, you never know :) So yes, I will add a check.

[frankenjoe]

The other option we have is to replace **kwargs with a dictionary. Then there is no conflict with the two positional arguments.