)]}'
{"sushy/resources/base.py":[{"author":{"_account_id":12597,"name":"Lin Yang","email":"lin.a.yang@intel.com","username":"lin.yang"},"change_message_id":"eedd24ac65f80f921b00cb1b99f560b6888311da","unresolved":false,"context_lines":[{"line_number":156,"context_line":"class FieldList(CompositeField):"},{"line_number":157,"context_line":"    \"\"\"Base class for fields consisting of a list of several sub-fields.\"\"\""},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"    def __init__(self, *args, **kwargs):"},{"line_number":160,"context_line":"        super(FieldList, self).__init__(*args, **kwargs)"},{"line_number":161,"context_line":""},{"line_number":162,"context_line":"    def _load(self, body, resource, nested_in\u003dNone):"}],"source_content_type":"text/x-python","patch_set":1,"id":"7f287b81_cae76f12","line":159,"updated":"2017-08-24 23:20:46.000000000","message":"No need __init__() here, since it doesn\u0027t have any additional logic, but only invoke parent class __init__.","commit_id":"9de4ff29fc3c2096f8abbfb6f67d8fdbe8286873"},{"author":{"_account_id":17275,"name":"Nate Potter","email":"nathaniel.potter@intel.com","username":"npotter"},"change_message_id":"7483c36d8c9217467cc53022fe660bb60366a04a","unresolved":false,"context_lines":[{"line_number":156,"context_line":"class FieldList(CompositeField):"},{"line_number":157,"context_line":"    \"\"\"Base class for fields consisting of a list of several sub-fields.\"\"\""},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"    def __init__(self, *args, **kwargs):"},{"line_number":160,"context_line":"        super(FieldList, self).__init__(*args, **kwargs)"},{"line_number":161,"context_line":""},{"line_number":162,"context_line":"    def _load(self, body, resource, nested_in\u003dNone):"}],"source_content_type":"text/x-python","patch_set":1,"id":"7f287b81_72d61ec6","line":159,"in_reply_to":"7f287b81_cae76f12","updated":"2017-08-25 15:46:16.000000000","message":"Done, thanks Lin. This was here from when I was experimenting with changing some stuff in the init and I forgot to remove it.","commit_id":"9de4ff29fc3c2096f8abbfb6f67d8fdbe8286873"},{"author":{"_account_id":14760,"name":"John L. Villalovos","email":"openstack.org@sodarock.com","username":"jlvillal"},"change_message_id":"045bf4e0435b7a3d8715e1081c87acf0d26423b1","unresolved":false,"context_lines":[{"line_number":153,"context_line":"        return iter(self._subfields)"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":""},{"line_number":156,"context_line":"class FieldList(CompositeField):"},{"line_number":157,"context_line":"    \"\"\"Base class for fields consisting of a list of several sub-fields.\"\"\""},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"    def _load(self, body, resource, nested_in\u003dNone):"}],"source_content_type":"text/x-python","patch_set":2,"id":"7f287b81_2840fba4","line":156,"updated":"2017-08-28 17:51:44.000000000","message":"Should this be ListField(...) to match up with other naming?","commit_id":"6299a826c52469f93c7faf3112addc49379cacf1"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"19704c6cae4b7a80319bcf0753b9a85380f94798","unresolved":false,"context_lines":[{"line_number":153,"context_line":"        return iter(self._subfields)"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":""},{"line_number":156,"context_line":"class FieldList(CompositeField):"},{"line_number":157,"context_line":"    \"\"\"Base class for fields consisting of a list of several sub-fields.\"\"\""},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"    def _load(self, body, resource, nested_in\u003dNone):"}],"source_content_type":"text/x-python","patch_set":2,"id":"7f515b1d_570ab7d1","line":156,"updated":"2017-10-09 19:17:29.000000000","message":"The way it\u0027s implemented, I\u0027d say it\u0027s a ListField, but I don\u0027t care too much. What I think is not right here is inheriting from CompositeField. ListField (or FieldList) is not a composite field itself. Only __init__ is actually usable (and can be copy-pasted), while neither _load nor the mapping interface functions apply here.\n\nI wonder if the right way to use this is\n\n class MyCompositeField(CompositeField):\n     foo \u003d Field(\u0027Foo\u0027)\n\n class Parent(Resource):\n     my_list \u003d FieldList(\u0027List\u0027, item\u003dMyCompositeField)\n\nthis may be tricky to implement, though. Note that in this case it\u0027s a FieldList indeed","commit_id":"6299a826c52469f93c7faf3112addc49379cacf1"},{"author":{"_account_id":17275,"name":"Nate Potter","email":"nathaniel.potter@intel.com","username":"npotter"},"change_message_id":"eb7e1dd61876677fc3517e0306e6d66517aa41e6","unresolved":false,"context_lines":[{"line_number":153,"context_line":"        return iter(self._subfields)"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":""},{"line_number":156,"context_line":"class FieldList(CompositeField):"},{"line_number":157,"context_line":"    \"\"\"Base class for fields consisting of a list of several sub-fields.\"\"\""},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"    def _load(self, body, resource, nested_in\u003dNone):"}],"source_content_type":"text/x-python","patch_set":2,"id":"7f287b81_8b2ec98f","line":156,"in_reply_to":"7f287b81_2840fba4","updated":"2017-08-28 18:12:37.000000000","message":"I thought about that, but with CompositeField it\u0027s a Field that is Composite, but this is a list of fields not a field that is a list, so it seemed to make more sense to call it field list. I\u0027d be open to changing it if folks think ListField makes more sense, but I personally think this way does.","commit_id":"6299a826c52469f93c7faf3112addc49379cacf1"},{"author":{"_account_id":16635,"name":"deray","email":"debayan.ray@gmail.com","username":"deray"},"change_message_id":"a028759a9f45097b4c26510548d013c2de2f6268","unresolved":false,"context_lines":[{"line_number":153,"context_line":"        return iter(self._subfields)"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":""},{"line_number":156,"context_line":"class FieldList(CompositeField):"},{"line_number":157,"context_line":"    \"\"\"Base class for fields consisting of a list of several sub-fields.\"\"\""},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"    def _load(self, body, resource, nested_in\u003dNone):"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f4e5783_e2170445","line":156,"in_reply_to":"7f515b1d_570ab7d1","updated":"2017-10-11 19:34:39.000000000","message":"+++++\nI favour this idea more.","commit_id":"6299a826c52469f93c7faf3112addc49379cacf1"},{"author":{"_account_id":17275,"name":"Nate Potter","email":"nathaniel.potter@intel.com","username":"npotter"},"change_message_id":"dcff672827b64b20573cd52e705e8c06ee891ad1","unresolved":false,"context_lines":[{"line_number":153,"context_line":"        return iter(self._subfields)"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":""},{"line_number":156,"context_line":"class FieldList(CompositeField):"},{"line_number":157,"context_line":"    \"\"\"Base class for fields consisting of a list of several sub-fields.\"\"\""},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"    def _load(self, body, resource, nested_in\u003dNone):"}],"source_content_type":"text/x-python","patch_set":2,"id":"7f515b1d_e691ef53","line":156,"in_reply_to":"7f515b1d_570ab7d1","updated":"2017-10-09 22:44:02.000000000","message":"Fair enough, I can definitely change the name to ListField as well. Thanks!","commit_id":"6299a826c52469f93c7faf3112addc49379cacf1"},{"author":{"_account_id":17275,"name":"Nate Potter","email":"nathaniel.potter@intel.com","username":"npotter"},"change_message_id":"d837e8907a78b83e835423ac01a4480e5228b2a9","unresolved":false,"context_lines":[{"line_number":153,"context_line":"        return iter(self._subfields)"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":""},{"line_number":156,"context_line":"class FieldList(CompositeField):"},{"line_number":157,"context_line":"    \"\"\"Base class for fields consisting of a list of several sub-fields.\"\"\""},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"    def _load(self, body, resource, nested_in\u003dNone):"}],"source_content_type":"text/x-python","patch_set":2,"id":"5f4e5783_7d24fb0d","line":156,"in_reply_to":"7f515b1d_e691ef53","updated":"2017-10-10 21:46:52.000000000","message":"I\u0027ve updated it to change the name and inherit from Field, I think that inclding the subfields value in the init still makes sense even if there\u0027s just one, as it creates a collection of objects that should be in the list.","commit_id":"6299a826c52469f93c7faf3112addc49379cacf1"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"19704c6cae4b7a80319bcf0753b9a85380f94798","unresolved":false,"context_lines":[{"line_number":165,"context_line":"        :returns: a new list object containing subfields."},{"line_number":166,"context_line":"        \"\"\""},{"line_number":167,"context_line":"        nested_in \u003d (nested_in or []) + self._path"},{"line_number":168,"context_line":"        values \u003d Field._load(self, body, resource)"},{"line_number":169,"context_line":"        if values is None:"},{"line_number":170,"context_line":"            return None"},{"line_number":171,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"7f515b1d_370bbbd5","line":168,"updated":"2017-10-09 19:17:29.000000000","message":"this is probably the main reason why I think this should not inherit from CompositeField. This really should be \n\n super(ListField, self)._load(body, resource)","commit_id":"6299a826c52469f93c7faf3112addc49379cacf1"},{"author":{"_account_id":17275,"name":"Nate Potter","email":"nathaniel.potter@intel.com","username":"npotter"},"change_message_id":"dcff672827b64b20573cd52e705e8c06ee891ad1","unresolved":false,"context_lines":[{"line_number":165,"context_line":"        :returns: a new list object containing subfields."},{"line_number":166,"context_line":"        \"\"\""},{"line_number":167,"context_line":"        nested_in \u003d (nested_in or []) + self._path"},{"line_number":168,"context_line":"        values \u003d Field._load(self, body, resource)"},{"line_number":169,"context_line":"        if values is None:"},{"line_number":170,"context_line":"            return None"},{"line_number":171,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"7f515b1d_86a11bac","line":168,"in_reply_to":"7f515b1d_370bbbd5","updated":"2017-10-09 22:44:02.000000000","message":"This makes sense, I\u0027ll look into ways that I can have this inherit from Field rather than CompositeField.","commit_id":"6299a826c52469f93c7faf3112addc49379cacf1"},{"author":{"_account_id":17275,"name":"Nate Potter","email":"nathaniel.potter@intel.com","username":"npotter"},"change_message_id":"d837e8907a78b83e835423ac01a4480e5228b2a9","unresolved":false,"context_lines":[{"line_number":165,"context_line":"        :returns: a new list object containing subfields."},{"line_number":166,"context_line":"        \"\"\""},{"line_number":167,"context_line":"        nested_in \u003d (nested_in or []) + self._path"},{"line_number":168,"context_line":"        values \u003d Field._load(self, body, resource)"},{"line_number":169,"context_line":"        if values is None:"},{"line_number":170,"context_line":"            return None"},{"line_number":171,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"5f4e5783_5d213ffb","line":168,"in_reply_to":"7f515b1d_86a11bac","updated":"2017-10-10 21:46:52.000000000","message":"Done","commit_id":"6299a826c52469f93c7faf3112addc49379cacf1"},{"author":{"_account_id":16635,"name":"deray","email":"debayan.ray@gmail.com","username":"deray"},"change_message_id":"a028759a9f45097b4c26510548d013c2de2f6268","unresolved":false,"context_lines":[{"line_number":153,"context_line":"        return iter(self._subfields)"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":""},{"line_number":156,"context_line":"class ListField(Field):"},{"line_number":157,"context_line":"    \"\"\"Base class for fields consisting of a list of several sub-fields.\"\"\""},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"    def __init__(self, *args, **kwargs):"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f4e5783_c20528c4","line":156,"updated":"2017-10-11 19:34:39.000000000","message":"I would say I rather favour the approach of being implemented this as ``FieldList``. I find the idea of using ``item_type`` as argument referring to the CompositeField more natural compared to this approach. That can be something of this sort::\n\n    class FieldList(Field):\n\n      def __init__(self, field, item_type, required\u003dFalse, default\u003dNone):\n        if not issubclass(item_type, CompositeField):\n            raise TypeError(\"The item_type argument must be a CompositeField\")\n\n        super(FieldList, self).__init__(\n            field, required\u003drequired, default\u003ddefault, adapter\u003dlist)\n        self.item_type \u003d item_type\n\n      def _load(self, body, resource, nested_in\u003dNone):\n        values \u003d super(FieldList, self)._load(body, resource, nested_in)\n        if values is None:\n            return None\n\n        items \u003d []\n        for value in values:\n            item \u003d self.item_type(\u0027\u0027)  # TBH no need to parse.\n            for attr, field in item._subfields.items():\n                setattr(item, attr, field._load(value, resource))\n\n            items.append(item)\n\n        return items\n\nAnd the way it can be used::\n\n\t\"List\": [\n\t  {\"iSCSI\": 1,\n\t   \"String\": \u0027a third string\u0027\n\t  },\n\t  {\"iSCSI\": 2,\n\t   \"String\": \u0027a fourth string\u0027,\n\t   \"Complex\": {\"integer\": 4, \"string\": \"hey\"}\n\t  }\n\t],\n\n\tclass ComplexCompositeField(resource_base.CompositeField):\n\t  integer \u003d resource_base.Field(\u0027integer\u0027)\n\t  string \u003d resource_base.Field(\u0027string\u0027)\n\n\tclass MyCompositeField(resource_base.CompositeField):\n\t  iscsi \u003d resource_base.Field(\u0027iSCSI\u0027, required\u003dTrue)\n\t  string \u003d resource_base.Field(\u0027String\u0027)\n\t  complex \u003d ComplexCompositeField(\u0027Complex\u0027)\n\n\tclass Resource(ResourceBase):\n\t  list \u003d base.FieldList(\u0027List\u0027, item_type\u003dMyCompositeField)\n\nAnd the means of referring to the values remain the same though::\n\n\t1 \u003d\u003d resource.list[0].iscsi\n\t2 \u003d\u003d resource.list[1].iscsi\n\t\u0027a third string\u0027 \u003d\u003d resource.list[0].string\n\t\u0027a fourth string\u0027 \u003d\u003d resource.list[1].string\n\tNone \u003d\u003d resource.list[0].complex\n\t4 \u003d\u003d resource.list[1].complex.integer\n\t\u0027hey\u0027 \u003d\u003d resource.list[1].complex.string","commit_id":"3220598e39dc5d4019daa7d36aa0aef2ce62e44e"},{"author":{"_account_id":16635,"name":"deray","email":"debayan.ray@gmail.com","username":"deray"},"change_message_id":"5c5de42df3355ef4961f8df8aea23d3c39ea7a07","unresolved":false,"context_lines":[{"line_number":153,"context_line":"        return iter(self._subfields)"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":""},{"line_number":156,"context_line":"class ListField(Field):"},{"line_number":157,"context_line":"    \"\"\"Base class for fields consisting of a list of several sub-fields.\"\"\""},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"    def __init__(self, *args, **kwargs):"}],"source_content_type":"text/x-python","patch_set":3,"id":"3f4b6375_4b15b29b","line":156,"in_reply_to":"5f4e5783_21083990","updated":"2017-10-23 08:49:44.000000000","message":"\u003e I also think that it\u0027s a little more inconvenient to have to create other composite classes and then pass them in to a field list rather than creating a new class that inherits from field list and includes the items in the list as members like a composite class would though - it adds extra steps and forces you to create more classes for things that wouldn\u0027t otherwise need their own class.\n\nI guess I sense a little disconnect in the understanding of the implementation I proposed here. Let\u0027s take the class representing the container of items of the element of the list as a new class. And this new class has to be created invariably of which approach you take up. In my proposed case it is: ``MyCompositeField`` (derived from ``CompositeField``) and in the implemented case it is: ``TestListField`` (derived from ``ListField``) [0]. I don\u0027t fathom any extra steps involved which forces to create more classes while comparing these 2 approaches. But of course, when you have a composite field stuff (container of multiple attributes) inside the list element itself, like ``Complex`` field, in:\n\n    {\"iSCSI\": 2,\n     \"String\": \u0027a fourth string\u0027,\n     \"Complex\": {\"integer\": 4, \"string\": \"hey\"}\n    }\n\nthen you will have to create another class (in both the approaches) to accommodate the dot access of the attributes of the composite field (e.g. ``integer``). Help me understand if you thought about any other extra steps involved which I couldn\u0027t.\n\n[0] https://review.openstack.org/#/c/495443/3/sushy/tests/unit/resources/test_base.py@168\n\n\u003e Why do you think that including an extra composite class is inherently more natural than using the _collect_fields function again in this class?\n\nThat new composite class which gets created and that solely intends to contain the attributes of the list element (and also facilitates the dot access of the attributes) is not an extra step, IMHO. A similar \u0027extra\u0027 class (the sublass of the ``ListField``, i.e. ``TestListField``) has to be created (just to contain the list element attributes) even in case of the approach so implemented in this patch. It has to be separate for each and every field which is a list and whose elements are not related.\n\nFrom my perspective the reasoning behind the naturality is of course the more promptness of the composing nature compared to subclass-ing while programming. I probably wont be able to give you a solid argument in this context, but will try to provide one advantageous use case which just comes on top of my head. Lets take this example where a distinct list-value attribute contains 2 separate distinct elements in the same list value based on some type property (the type property could be outside or inside the list element value). When outside, the response could be one of these 2 at any given time:\n\n\n    item_type \u003d \"dash-separated\",\n    \"List\": [\n      {\"attr-1\": \"val01\",\n       \"attr-2\": \"val02\",\n      },\n      {\"attr-1\": \"val11\",\n       \"attr-2\": \"val12\",\n      },\n    ]\n\nOR\n\t\n    item_type \u003d \"underscore-separated\",\n    \"List\": [\n      {\"attr_1\": \"value01\",\n       \"attr_2\": \"value02\",\n      },\n      {\"attr_1\": \"value11\",\n       \"attr_2\": \"value12\",\n      },\n    ]\n\nThen you can define the list attribute as:\n\n    class Resource(ResourceBase):\n      list \u003d base.FieldList(\u0027List\u0027, item_types[\n        MyDashSeparatedCompositeField,\n        MyUnderscoreSeparatedCompositeField])\n\nor something similar. I find it difficult to envisage how to accommodate that in ``ListField`` approach. Not that you can\u0027t do it tho:\n\n    class Resource(ResourceBase):\n      list \u003d base.ListFieldWrapper(\n        MyDashSeparatedListField(\u0027List\u0027),\n        MyUnderscoreSeparatedListField(\u0027List\u0027))\n\nBut that complicates from the user\u0027s PoV. She needs to remember another Field type (here the wrapper class). Again, when the ``item_type`` is present inside the element value (i.e. the list elements are supposed to be heterogeneous in nature) then the situation turns out to be even more complex. But in the ``FieldList`` you can still handle this by some minor tricky tweaking.\n\nPlease consider the depicted case as a hypothetical one and that too used for illustration purpose only. Probably there wont be any such scenarios encountered in real life. Please do let me know what you think.","commit_id":"3220598e39dc5d4019daa7d36aa0aef2ce62e44e"},{"author":{"_account_id":17275,"name":"Nate Potter","email":"nathaniel.potter@intel.com","username":"npotter"},"change_message_id":"35a94c519a17fc8a863051cfcdc8f177e003b45f","unresolved":false,"context_lines":[{"line_number":153,"context_line":"        return iter(self._subfields)"},{"line_number":154,"context_line":""},{"line_number":155,"context_line":""},{"line_number":156,"context_line":"class ListField(Field):"},{"line_number":157,"context_line":"    \"\"\"Base class for fields consisting of a list of several sub-fields.\"\"\""},{"line_number":158,"context_line":""},{"line_number":159,"context_line":"    def __init__(self, *args, **kwargs):"}],"source_content_type":"text/x-python","patch_set":3,"id":"5f4e5783_21083990","line":156,"in_reply_to":"5f4e5783_c20528c4","updated":"2017-10-13 18:03:22.000000000","message":"I think that solution looks good, but I also think that it\u0027s a little more inconvenient to have to create other composite classes and then pass them in to a field list rather than creating a new class that inherits from field list and includes the items in the list as members like a composite class would though - it adds extra steps and forces you to create more classes for things that wouldn\u0027t otherwise need their own class. Why do you think that including an extra composite class is inherently more natural than using the _collect_fields function again in this class?","commit_id":"3220598e39dc5d4019daa7d36aa0aef2ce62e44e"}]}