)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":13689,"name":"Hironori Shiina","email":"Hironori.Shiina@fujitsu.com","username":"shiina"},"change_message_id":"483c2bab8d34f2611b4ada0c2bcffc6a450b02e1","unresolved":false,"context_lines":[{"line_number":25,"context_line":"the \u0027fields\u0027 query parameter, but the marker/UUID was incorrect."},{"line_number":26,"context_line":""},{"line_number":27,"context_line":"Change-Id: I1309fb504f92c1ab3586c3abf4fc5d0e7ccbbd7d"},{"line_number":28,"context_line":"Closes-Bug: #1715541"},{"line_number":29,"context_line":"Closes-Bug: #1718726"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"7f515b1d_4bd9047d","line":28,"updated":"2017-09-27 16:36:42.000000000","message":"Partial-Bug might be better since similar fixes are necessary for other resources to close the bug completely.","commit_id":"640d429f03eab40e9e08e37dc071a1029474db06"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"74109b4bc7e4dcbff39847760c37a54ef93d2509","unresolved":false,"context_lines":[{"line_number":25,"context_line":"the \u0027fields\u0027 query parameter, but the marker/UUID was incorrect."},{"line_number":26,"context_line":""},{"line_number":27,"context_line":"Change-Id: I1309fb504f92c1ab3586c3abf4fc5d0e7ccbbd7d"},{"line_number":28,"context_line":"Closes-Bug: #1715541"},{"line_number":29,"context_line":"Closes-Bug: #1718726"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"7f515b1d_8b549c22","line":28,"in_reply_to":"7f515b1d_4bd9047d","updated":"2017-09-27 16:52:32.000000000","message":"Oh, you\u0027re right. I thought this bug was similar to the duplicated bug 1717880, which was just for the node case, but this is for all of them.","commit_id":"640d429f03eab40e9e08e37dc071a1029474db06"}],"ironic/api/controllers/v1/node.py":[{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"976b40c29bfb23bf895e1affac471bbb0ae6095f","unresolved":false,"context_lines":[{"line_number":1318,"context_line":"                              specified_fields\u003dFalse):"},{"line_number":1319,"context_line":"        \"\"\"Get the nodes."},{"line_number":1320,"context_line":""},{"line_number":1321,"context_line":"        This gets the information for the requested nodes,"},{"line_number":1322,"context_line":""},{"line_number":1323,"context_line":"        :param chassis_uuid: Optional UUID of a chassis, to get only nodes for"},{"line_number":1324,"context_line":"                             that chassis."}],"source_content_type":"text/x-python","patch_set":6,"id":"7f515b1d_b44d539b","line":1321,"updated":"2017-10-02 13:15:27.000000000","message":"s/,/./","commit_id":"d6797e505fcef8762aec50ce0bab5beb00498df3"},{"author":{"_account_id":6637,"name":"Sam Betts","email":"sam@code-smash.net","username":"sambetts"},"change_message_id":"141cab7c711d5ec8561a1e9bd9fdf9314422bd08","unresolved":false,"context_lines":[{"line_number":1061,"context_line":"            # collection.nodes.)"},{"line_number":1062,"context_line":"            marker_uuid \u003d nodes[-1].uuid"},{"line_number":1063,"context_line":"        collection.next \u003d collection.get_next(limit, url\u003durl,"},{"line_number":1064,"context_line":"                                              marker\u003dmarker_uuid, **kwargs)"},{"line_number":1065,"context_line":"        return collection"},{"line_number":1066,"context_line":""},{"line_number":1067,"context_line":"    @classmethod"}],"source_content_type":"text/x-python","patch_set":7,"id":"df87a7cf_06068ecf","line":1064,"updated":"2017-12-06 14:44:21.000000000","message":"Don\u0027t we need to pass the fields in here too, so the .next also has the fields\u003d query added?","commit_id":"d4c520d33043a5416fb62d9fd2987be8127e81e6"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"a63a1f55b124ef9507142b89ccd205650d7bab1a","unresolved":false,"context_lines":[{"line_number":1061,"context_line":"            # collection.nodes.)"},{"line_number":1062,"context_line":"            marker_uuid \u003d nodes[-1].uuid"},{"line_number":1063,"context_line":"        collection.next \u003d collection.get_next(limit, url\u003durl,"},{"line_number":1064,"context_line":"                                              marker\u003dmarker_uuid, **kwargs)"},{"line_number":1065,"context_line":"        return collection"},{"line_number":1066,"context_line":""},{"line_number":1067,"context_line":"    @classmethod"}],"source_content_type":"text/x-python","patch_set":7,"id":"df87a7cf_292cc397","line":1064,"in_reply_to":"df87a7cf_06068ecf","updated":"2017-12-06 15:02:13.000000000","message":"We do. See L1401. (Let me know if I should add some comment or something to make it more obvious...)","commit_id":"d4c520d33043a5416fb62d9fd2987be8127e81e6"},{"author":{"_account_id":6637,"name":"Sam Betts","email":"sam@code-smash.net","username":"sambetts"},"change_message_id":"0438e91ae4a2e2357b66c5316010aa3e0730f6f7","unresolved":false,"context_lines":[{"line_number":1061,"context_line":"            # collection.nodes.)"},{"line_number":1062,"context_line":"            marker_uuid \u003d nodes[-1].uuid"},{"line_number":1063,"context_line":"        collection.next \u003d collection.get_next(limit, url\u003durl,"},{"line_number":1064,"context_line":"                                              marker\u003dmarker_uuid, **kwargs)"},{"line_number":1065,"context_line":"        return collection"},{"line_number":1066,"context_line":""},{"line_number":1067,"context_line":"    @classmethod"}],"source_content_type":"text/x-python","patch_set":7,"id":"df87a7cf_2935a3f7","line":1064,"in_reply_to":"df87a7cf_292cc397","updated":"2017-12-06 15:38:40.000000000","message":"ah whoops missed that, sooo thats why you\u0027ve renamed the fields parameter for this function! not sure I\u0027m a massive fan of passing two kwargs into the function essentially representing the exactly same data.\n\nman this code is a mess relying on **kwargs :( \n\npersonally I think I\u0027d prefer to remove fields_in_response from this function and replace it with kwargs.get(\u0027fields\u0027) which will make it more obvious that its getting passed through.","commit_id":"d4c520d33043a5416fb62d9fd2987be8127e81e6"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"e26a58e4721c211bdd13aca939d3518fc9ebf813","unresolved":false,"context_lines":[{"line_number":1061,"context_line":"            # collection.nodes.)"},{"line_number":1062,"context_line":"            marker_uuid \u003d nodes[-1].uuid"},{"line_number":1063,"context_line":"        collection.next \u003d collection.get_next(limit, url\u003durl,"},{"line_number":1064,"context_line":"                                              marker\u003dmarker_uuid, **kwargs)"},{"line_number":1065,"context_line":"        return collection"},{"line_number":1066,"context_line":""},{"line_number":1067,"context_line":"    @classmethod"}],"source_content_type":"text/x-python","patch_set":7,"id":"df87a7cf_eca2694b","line":1064,"in_reply_to":"df87a7cf_2935a3f7","updated":"2017-12-06 16:08:51.000000000","message":"well, we can\u0027t do that easily. Because the \u0027fields\u0027 query parameter value is not necessarily equal to the \u0027fields\u0027 in the response. See L1541 :-( It would be wrong to tack on those fields in the query part of the URL if the user didn\u0027t actually specify them.\n\nWhich doesn\u0027t mean that we can\u0027t do what you want, by eg moving down the \u0027fields in the response\u0027 logic to here, but I don\u0027t actually think that makes sense, and I don\u0027t really want to touch that much of the code.\n\nI think it is good to have the distinction between the fields in the response and the fields in the url\u0027s query part.\n\nI could put in a comment about that somewhere if you have a suggestion...","commit_id":"d4c520d33043a5416fb62d9fd2987be8127e81e6"},{"author":{"_account_id":6637,"name":"Sam Betts","email":"sam@code-smash.net","username":"sambetts"},"change_message_id":"462caf1407ee5fba45eaf78038d1d1afc63d2570","unresolved":false,"context_lines":[{"line_number":1061,"context_line":"            # collection.nodes.)"},{"line_number":1062,"context_line":"            marker_uuid \u003d nodes[-1].uuid"},{"line_number":1063,"context_line":"        collection.next \u003d collection.get_next(limit, url\u003durl,"},{"line_number":1064,"context_line":"                                              marker\u003dmarker_uuid, **kwargs)"},{"line_number":1065,"context_line":"        return collection"},{"line_number":1066,"context_line":""},{"line_number":1067,"context_line":"    @classmethod"}],"source_content_type":"text/x-python","patch_set":7,"id":"df87a7cf_f7b8f6c8","line":1064,"in_reply_to":"df87a7cf_eca2694b","updated":"2017-12-06 16:29:14.000000000","message":"man I hate this code sometimes... perhaps something like:\n\n  if fields \u003d\u003d _DEFAULT_RETURN_FIELDS:\n     kwargs.pop(\u0027fields\u0027)\n\nto avoid needing to pass around if the fields were specified or not.\n\nI also just realised I think we\u0027re missing including the fields\u003d query in the individual links for the nodes in the collection (if we want it to do that) and if we were to do that then we\u0027d need logic similar to above in the _convert_with_links function anyway.","commit_id":"d4c520d33043a5416fb62d9fd2987be8127e81e6"},{"author":{"_account_id":6637,"name":"Sam Betts","email":"sam@code-smash.net","username":"sambetts"},"change_message_id":"6d2bba287135ddd7c7c2a0ea010a94fc71110e3f","unresolved":false,"context_lines":[{"line_number":1061,"context_line":"            # collection.nodes.)"},{"line_number":1062,"context_line":"            marker_uuid \u003d nodes[-1].uuid"},{"line_number":1063,"context_line":"        collection.next \u003d collection.get_next(limit, url\u003durl,"},{"line_number":1064,"context_line":"                                              marker\u003dmarker_uuid, **kwargs)"},{"line_number":1065,"context_line":"        return collection"},{"line_number":1066,"context_line":""},{"line_number":1067,"context_line":"    @classmethod"}],"source_content_type":"text/x-python","patch_set":7,"id":"df87a7cf_5db46135","line":1064,"in_reply_to":"df87a7cf_f20564c3","updated":"2017-12-06 17:21:02.000000000","message":"My suggestion is:\n\n def convert_with_links(nodes, limit, url\u003dNone, **kwargs):\n    collection \u003d NodeCollection()\n    collection.nodes \u003d [\n        Node.convert_with_links(n, fields\u003dkwargs.get(\u0027fields\u0027))\n        for n in nodes]\n    marker_uuid \u003d None\n    if nodes:\n        # The UUID of the last node in the list, needed for generating\n        # the \u0027next\u0027 link. (This is to handle the case where the response\n        # doesn\u0027t include the UUIDs so the info is not available in\n        # collection.nodes.)\n        marker_uuid \u003d nodes[-1].uuid\n    if kwargs.get(\u0027fields\u0027) \u003d\u003d _DEFAULT_RETURN_FIELDS:\n        # Fields are the default set, so there is no need to\n        # include it in the URL\n        kwargs.pop(\u0027fields\u0027)\n    collection.next \u003d collection.get_next(limit, url\u003durl,\n                                          marker\u003dmarker_uuid, **kwargs)\n\ncould even do:\n\n def convert_with_links(nodes, limit, url\u003dNone, fields\u003dNone, **kwargs):\n     ...\n     if fields !\u003d _DEFAULT_RETURN_FIELDS:\n         kwargs[\u0027fields\u0027] \u003d fields\n\nalthough I think the first option is clearer.\n\nAlso perhaps instead of setting fields to the default set of fields in the API function below we should move that logic into the convert with links function above, so its like:\n\ndef _convert_with_links(..., fields\u003dNone):\n  if fields is None:\n      fields \u003d _DEFAULT_RETURN_FIELDS","commit_id":"d4c520d33043a5416fb62d9fd2987be8127e81e6"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"99b46949bd6374a76a4ae6e743bff1786474a1e1","unresolved":false,"context_lines":[{"line_number":1061,"context_line":"            # collection.nodes.)"},{"line_number":1062,"context_line":"            marker_uuid \u003d nodes[-1].uuid"},{"line_number":1063,"context_line":"        collection.next \u003d collection.get_next(limit, url\u003durl,"},{"line_number":1064,"context_line":"                                              marker\u003dmarker_uuid, **kwargs)"},{"line_number":1065,"context_line":"        return collection"},{"line_number":1066,"context_line":""},{"line_number":1067,"context_line":"    @classmethod"}],"source_content_type":"text/x-python","patch_set":7,"id":"df87a7cf_f20564c3","line":1064,"in_reply_to":"df87a7cf_f7b8f6c8","updated":"2017-12-06 16:47:41.000000000","message":"I don\u0027t care about passing around if the fields were specified or not. I do care that the code is written so that it is clearer/easy to understand, so that it can be maintained, etc. I\u0027m not convinced that your suggestion will make it easier; it seems like it\u0027ll make it harder. (Actually, I\u0027m not quite sure I understand your suggestion.)\n\nI don\u0027t like this code either, but I also don\u0027t have time to try to clean it up. Which is perhaps how we got to this point :-(\n\nFor me, since I worked on it, the code seems clean, separating the fields in the response from the fields in the url\u0027s query, since they are not necessarily the same. And keeping the query-fields in kwargs along with any other query-fields is consistent too.\n\nI\u0027m not sure it is broken for an individual node. If it is, that is a separate patch. I think there is too much going on here, to also address that. (I just skimmed that code, ugh, I think it might be ok, but I\u0027ll verify/test later...)","commit_id":"d4c520d33043a5416fb62d9fd2987be8127e81e6"}]}