Allow update #14

PhilippHomann · 2021-07-16T08:26:48Z

Follow-up of #13

vsoch

The tests look okay - and I made some comments for the first part of the change. I'm honestly having trouble following the logic of the changes - if you could put more comments and description this would be helpful.

opencontainers/struct.py

vsoch · 2021-07-16T16:43:24Z

opencontainers/struct.py

+                child = self.attType[0]
+
+                # It's either a nested structure
+                if is_struct(child):


This could get hairy if we have multiple levels of nesting. Can you implement this possibly with recursion? Once we've grabbed a child (and we know it's a struct) does it not work to call the function again for that struct?

I've added a test for nested Structs.
Recursion is not needed as .load(value) will construct nested structs for us.

vsoch · 2021-07-16T16:45:18Z

opencontainers/struct.py

+                # It's either a nested structure
+                if is_struct(child):
+
+                    # If we have a list of values, generate them


The logic down here would change I think too. There aren't enough comments here to help me understand what is going on. (e.g., can we do like I did with case 1, case 2, etc.

opencontainers/struct.py

Struct.add() is now only supports updating a list or dict. Use .set() for setting a new value on an attribute.

vsoch · 2021-07-24T21:04:31Z

@PhilippHomann checking in - are you going to remove the last commit?

PhilippHomann · 2021-07-28T11:29:34Z

Which commit do you mean? Why do you want it to be removed?

vsoch · 2021-07-28T14:27:42Z

We discussed removing set: #14 (comment).

This reverts commit 3349754.

PhilippHomann · 2021-07-30T11:44:29Z

Reverted the last commit. Sorry for the delay!

opencontainers/struct.py

vsoch · 2021-07-30T16:13:37Z

opencontainers/struct.py

-                if isinstance(value, list):
-                    values = []
-                    for v in value:
+        if not is_struct(type(value)):


I'd like to still see a comment above this line (89) explaining what this block is going to cover.

vsoch · 2021-07-30T16:15:16Z

opencontainers/struct.py

+            # Transform value to Struct if not already
+            if not is_struct(valueType):
+                value = attr.attType().load(value)
+            if attr.attType in [StrStruct, IntStruct]:


Adding an int/string to another? is this the behavior we want?

I don't know.
In another comment (#14 (comment)) you noticed that replacing the value might not be the thing we want.
So maybe we should only allow dict and list as type for value?!

My thinking is that add should explicitly be for things you can add/append to (like list or dict). If someone wants to change a value entirely they can just use set. I can see the argument for wanting add for an int (to add a number to an existing one) but I'd say we should wait until someone actually speaks up about that use case.

But imho we should have a Struct.set() then.
Because for now one have to use Struct.attrs["name"].set("value") to change the value of an attribute.
But you asked me to revert the commit which added Struct.set()
So I don't understand what we're up to..

I thought we already have .set() on lines 75/84?

Indeed but only in StructAttr not in Struct.
So Struct.attrs["name"].set("value") is the only way to change a value of an attribute.
Don't know if this the way one should set a value when we have a Struct.add("name", "value") to append to a list.

Oh I see. I think Struct.add with name and value would make sense to append to a list, do you think not? Let's discuss what you think would be most intuitive!

vsoch · 2021-07-30T16:15:39Z

opencontainers/struct.py

+            if attr.attType in [StrStruct, IntStruct]:
+                if attr.value is not None:
+                    value = attr.value + value
+        elif attr.attType == list or isinstance(attr.attType, list):


Why do we need to check attType and the instance type?

This is needed because for attributes with attType=[IntStruct] the isinstance() check is True and for attributes with attType=list or attType=dict as used in opencontainers/image/v1/config.py.

vsoch · 2021-07-30T16:16:08Z

opencontainers/struct.py

+                # Load values from dict or list of dicts
+                # list may also already contain Structs
+                if valueType == dict:
+                    value = attr.attType[0]().load(value)


Do you see common functoinality between here and checking types and the above logic for set()?

vsoch · 2021-07-30T16:16:45Z

opencontainers/struct.py

+            else:
+                raise ValueError("dict expected for {}, got {}: {}".format(name, valueType, value))
+        else:
+            pass


Should there be some warning message here about an unknown type?

vsoch · 2021-07-30T16:17:08Z

opencontainers/struct.py

@@ -314,25 +352,28 @@ def load(self, content, validate=True):
        given a dictionary load into its respective object
        if validate is True, we require it to be completely valid.
        """
-        if not isinstance(content, dict):
-            bot.exit("Please provide a dictionary or list to load.")
+        if Struct in type(content).__bases__:


Can you add comments above this line to explain the logic here?

vsoch · 2021-07-30T16:17:42Z

opencontainers/struct.py

@@ -383,6 +424,12 @@ def validate(self):
                return False
        return True

+    def get(self, name, default=None):
+        r = self.attrs[name].value


If name isn't in self.attrs won't this error?

Co-authored-by: Vanessasaurus <814322+vsoch@users.noreply.github.com>

PhilippHomann · 2021-08-10T06:36:40Z

Changed my mind on using oci-python.
As I don't need these changes anymore, I'm closing the PR.
Feel free to adapt to my changes.

vsoch · 2021-08-10T06:38:25Z

Will do, thanks!

@vsoch

* adding support to update an existing value currently, if we take a structure and run "add" to update an attribute, it completely replaces any content there (e.g., a dict or list). But with add we would actually want it to update - if the user wants to replace they can use set. Signed-off-by: vsoch <vsoch@users.noreply.github.com> * Allow update (#14) * Add test for updating structs * Improve .add() to allow updating * Add .get() to Struct to receive value * Remove type hint to remain compatible * Add test to deeply nested structs * Really add value instead of replacing if type is int or str * Add Struct.set() Struct.add() is now only supports updating a list or dict. Use .set() for setting a new value on an attribute. * Revert "Add Struct.set()" This reverts commit 3349754. * Commit as suggested by @vsoch Co-authored-by: Vanessasaurus <814322+vsoch@users.noreply.github.com> Co-authored-by: Vanessasaurus <814322+vsoch@users.noreply.github.com> * run black and update docstrings Signed-off-by: vsoch <vsoch@users.noreply.github.com> * pin black Signed-off-by: vsoch <vsoch@users.noreply.github.com> Co-authored-by: vsoch <vsoch@users.noreply.github.com> Co-authored-by: PhilippHomann <3265572+PhilippHomann@users.noreply.github.com>

PhilippHomann and others added 3 commits July 16, 2021 09:58

Add test for updating structs

b37d190

Improve .add() to allow updating

7315edb

Add .get() to Struct to receive value

c82fc6d

vsoch reviewed Jul 16, 2021

View reviewed changes

PhilippHomann added 4 commits July 19, 2021 08:31

Remove type hint to remain compatible

ac6af0c

Add test to deeply nested structs

79738cf

Really add value instead of replacing if type is int or str

d586ea9

Add Struct.set()

3349754

Struct.add() is now only supports updating a list or dict. Use .set() for setting a new value on an attribute.

Revert "Add Struct.set()"

ab53243

This reverts commit 3349754.

vsoch reviewed Jul 30, 2021

View reviewed changes

Commit as suggested by @vsoch

b018bb9

Co-authored-by: Vanessasaurus <814322+vsoch@users.noreply.github.com>

PhilippHomann closed this Aug 10, 2021

vsoch reopened this Aug 10, 2021

vsoch merged commit ab24c49 into vsoch:allow-update Aug 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow update #14

Allow update #14

PhilippHomann commented Jul 16, 2021

vsoch left a comment

vsoch Jul 16, 2021

PhilippHomann Jul 19, 2021

vsoch Jul 16, 2021

vsoch commented Jul 24, 2021

PhilippHomann commented Jul 28, 2021

vsoch commented Jul 28, 2021

PhilippHomann commented Jul 30, 2021 •

edited

vsoch Jul 30, 2021

vsoch Jul 30, 2021

PhilippHomann Aug 2, 2021

vsoch Aug 2, 2021

PhilippHomann Aug 3, 2021

vsoch Aug 3, 2021

PhilippHomann Aug 3, 2021

vsoch Aug 3, 2021

vsoch Jul 30, 2021

PhilippHomann Aug 2, 2021

vsoch Jul 30, 2021

vsoch Jul 30, 2021

vsoch Jul 30, 2021

vsoch Jul 30, 2021

PhilippHomann commented Aug 10, 2021

vsoch commented Aug 10, 2021

Allow update #14

Allow update #14

Conversation

PhilippHomann commented Jul 16, 2021

vsoch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsoch commented Jul 24, 2021

PhilippHomann commented Jul 28, 2021

vsoch commented Jul 28, 2021

PhilippHomann commented Jul 30, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhilippHomann commented Aug 10, 2021

vsoch commented Aug 10, 2021

PhilippHomann commented Jul 30, 2021 •

edited