linkml / linkml

Linked Open Data Modeling Language
https://linkml.io/linkml
Other
326 stars 101 forks source link

Difficulties resetting multivalued slots that are inlined as dicts #370

Open cmungall opened 3 years ago

cmungall commented 3 years ago

All generated dataclasses inherit JsonObj from jsonasobj2, which overrides __setattr__ which turns dicts into JsonObjs

   def __setattr__(self, key, value):
        super().__setattr__(key, JsonObj(value) if isinstance(value, dict) else value)

This is convenient for instantiating objects as dicts, but for multivalued dict-inlined collections, we want these to be modeled as native dicts

Example from metamodel:

>>> c=ClassDefinition('foo')
>>> c.attributes = {}
>>> type(c.attributes)
<class 'jsonasobj2._jsonobj.JsonObj'>
>>> c.attributes.keys()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/cjm/.local/share/virtualenvs/linkml-6dchTQmP/lib/python3.9/site-packages/jsonasobj2/_jsonobj.py", line 160, in __getattr__
    return super().__getattribute__(item)
AttributeError: 'JsonObj' object has no attribute 'keys'
>>> c.attributes.keys()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/cjm/.local/share/virtualenvs/linkml-6dchTQmP/lib/python3.9/site-packages/jsonasobj2/_jsonobj.py", line 160, in __getattr__
    return super().__getattribute__(item)
AttributeError: 'JsonObj' object has no attribute 'keys'

I don't think this intentional - we want to be able to use dict methods here

A workaround is to instead use .clear() rather than explicitly setting to {}

e.g.

>>> c=ClassDefinition('foo')
>>> c.attributes.clear()
>>> type(c.attributes)
<class 'dict'>
>>> c.attributes['x'] = SlotDefinition('x')
>>> c.attributes.keys()
dict_keys(['x'])

However, this is a little ununtuitive, it seems we should be able to set directly

It also means it is inconvenient to do things like merge dicts using the idiom

c.attributes = {**atts1, **att2s}

instead we should do

c.attributes.clear()
c.attributes.update({**atts1, **att2s})

which is fine, a minor inconvenient, but the lack of direct setting ability will cause confusing errors

sierra-moxon commented 2 years ago

Still needs more work; still a relevant issue.