)]}'
{"oslo_utils/specs_matcher.py":[{"author":{"_account_id":18893,"name":"Miles Gould","email":"mgould@redhat.com","username":"pozorvlak"},"change_message_id":"ee21e84b07e4061d6b8d99c11de16c77b20a689d","unresolved":false,"context_lines":[{"line_number":66,"context_line":"        return False"},{"line_number":67,"context_line":""},{"line_number":68,"context_line":"    if op \u003d\u003d \u0027\u003call-in\u003e\u0027:"},{"line_number":69,"context_line":"        if isinstance(value, str):"},{"line_number":70,"context_line":"            raise TypeError(\"\u003call-in\u003e expressions require value to be a \""},{"line_number":71,"context_line":"                            \"non-string iterable\")"},{"line_number":72,"context_line":"        return method(list(value), words)"}],"source_content_type":"text/x-python","patch_set":1,"id":"1aa78d24_d15e7b11","line":69,"updated":"2016-07-08 15:01:57.000000000","message":"Maybe also check isinstance(value, Iterable)?","commit_id":"1a997c3e41939783239938e22410980c44243897"},{"author":{"_account_id":8688,"name":"Alexis Lee","email":"openstack@lxsli.co.uk","username":"lxsli"},"change_message_id":"e697b44859de434e5e6d6cece4db0bbb25d816ba","unresolved":false,"context_lines":[{"line_number":66,"context_line":"        return False"},{"line_number":67,"context_line":""},{"line_number":68,"context_line":"    if op \u003d\u003d \u0027\u003call-in\u003e\u0027:"},{"line_number":69,"context_line":"        if isinstance(value, str):"},{"line_number":70,"context_line":"            raise TypeError(\"\u003call-in\u003e expressions require value to be a \""},{"line_number":71,"context_line":"                            \"non-string iterable\")"},{"line_number":72,"context_line":"        return method(list(value), words)"}],"source_content_type":"text/x-python","patch_set":1,"id":"1aa78d24_cc53826b","line":69,"in_reply_to":"1aa78d24_d15e7b11","updated":"2016-07-08 15:24:51.000000000","message":"list() does that for me; but I suppose the error message would be nicer if I did","commit_id":"1a997c3e41939783239938e22410980c44243897"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"a2bf8490cc9db9255b75476ba492a19ee5094aef","unresolved":false,"context_lines":[{"line_number":94,"context_line":"                            \" iterable\" % ast[0])"},{"line_number":95,"context_line":"    elif not is_str:"},{"line_number":96,"context_line":"        raise TypeError(\"%s expressions require value to be a string\" % ast[0])"},{"line_number":97,"context_line":""},{"line_number":98,"context_line":"    op \u003d op_methods[ast[0]]"},{"line_number":99,"context_line":"    return op(cmp_value, *ast[1:])"}],"source_content_type":"text/x-python","patch_set":3,"id":"1aa78d24_a2419a0d","line":97,"updated":"2016-07-11 13:47:07.000000000","message":"I\u0027m not convinced this is the right approach/solution.\n\nLooking at how this code is used in nova, you can see that they call .match() with two strings [1]. I suspect that it was easier to do this, than to check if the second argument was \u0027\u0027\u003call-in\u003e\" before deciding to send the list or the str(list).\n\nMy guess is that we\u0027re in this mess due to lack of documentation. I think the solution is that if \u0027\u003call-in\u003e\u0027 is the operator, if cmp_value is a string, then unstringify it to get a list (or a single atom?).\n\nMy other thought is to wonder why nova sends a string for cmp_value, instead of sending whatever it has, and for this method to change the type (to stringify it if it needs to).\n\n[1] https://github.com/openstack/nova/blob/90ec46c87fbf572805c7758377431e26c93622a4/nova/scheduler/filters/compute_capabilities_filter.py#L87","commit_id":"58143fdcb79e641dd23a4dc78109dcedcf3530fd"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"8c512642dd59cd049037736f3d458d803630c676","unresolved":false,"context_lines":[{"line_number":94,"context_line":"                            \" iterable\" % ast[0])"},{"line_number":95,"context_line":"    elif not is_str:"},{"line_number":96,"context_line":"        raise TypeError(\"%s expressions require value to be a string\" % ast[0])"},{"line_number":97,"context_line":""},{"line_number":98,"context_line":"    op \u003d op_methods[ast[0]]"},{"line_number":99,"context_line":"    return op(cmp_value, *ast[1:])"}],"source_content_type":"text/x-python","patch_set":3,"id":"1aa78d24_04bfc7aa","line":97,"in_reply_to":"1aa78d24_30667e51","updated":"2016-07-11 20:17:38.000000000","message":"OR just remove \u003call-in\u003e operator for now, until we figure out the best way to address it?","commit_id":"58143fdcb79e641dd23a4dc78109dcedcf3530fd"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"802bbc50816a75be3bd97034f2a7ebf8a7c01b6f","unresolved":false,"context_lines":[{"line_number":94,"context_line":"                            \" iterable\" % ast[0])"},{"line_number":95,"context_line":"    elif not is_str:"},{"line_number":96,"context_line":"        raise TypeError(\"%s expressions require value to be a string\" % ast[0])"},{"line_number":97,"context_line":""},{"line_number":98,"context_line":"    op \u003d op_methods[ast[0]]"},{"line_number":99,"context_line":"    return op(cmp_value, *ast[1:])"}],"source_content_type":"text/x-python","patch_set":3,"id":"1aa78d24_e52a878f","line":97,"in_reply_to":"1aa78d24_a2419a0d","updated":"2016-07-11 15:14:57.000000000","message":"Thinking more about this...\n\nIt isn\u0027t clear to me that if you pass in a cmp_value that isn\u0027t a string, that it should break for all operators that are not \u003call-in\u003e. Do we think that match(3, \"\u003e\u003d 2\u0027) should raise an exception, but match(\u00273\u0027, \"\u003e\u003d 2\") will work?","commit_id":"58143fdcb79e641dd23a4dc78109dcedcf3530fd"},{"author":{"_account_id":8688,"name":"Alexis Lee","email":"openstack@lxsli.co.uk","username":"lxsli"},"change_message_id":"7ddd1dc839c8ec14e97477a668e64eb959abeefc","unresolved":false,"context_lines":[{"line_number":94,"context_line":"                            \" iterable\" % ast[0])"},{"line_number":95,"context_line":"    elif not is_str:"},{"line_number":96,"context_line":"        raise TypeError(\"%s expressions require value to be a string\" % ast[0])"},{"line_number":97,"context_line":""},{"line_number":98,"context_line":"    op \u003d op_methods[ast[0]]"},{"line_number":99,"context_line":"    return op(cmp_value, *ast[1:])"}],"source_content_type":"text/x-python","patch_set":3,"id":"1aa78d24_30667e51","line":97,"in_reply_to":"1aa78d24_b53d402d","updated":"2016-07-11 19:59:01.000000000","message":"Well... yeah... but I didn\u0027t design the interface, I\u0027m just trying to help users stick to it and thereby get the behaviour they expect.\n\nComments are definitely worthwhile, Josh volunteered to do some but if he doesn\u0027t beat me to it, I\u0027ll add some tomorrow when I\u0027m back at work.\n\nWe have option 4, where Nova doesn\u0027t allow \u003call-in\u003e at all and does all(match(v) for v in values). This has back-compat issues though, might have to rewrite some user-provided expressions?\n\nOption 1 would also be OK, maybe we should add an \u003cany-in\u003e which also takes a list and is like any(match(v) for v in values).","commit_id":"58143fdcb79e641dd23a4dc78109dcedcf3530fd"},{"author":{"_account_id":8688,"name":"Alexis Lee","email":"openstack@lxsli.co.uk","username":"lxsli"},"change_message_id":"a68c0641cbac37210b246bcbce2bd7b21d57241c","unresolved":false,"context_lines":[{"line_number":94,"context_line":"                            \" iterable\" % ast[0])"},{"line_number":95,"context_line":"    elif not is_str:"},{"line_number":96,"context_line":"        raise TypeError(\"%s expressions require value to be a string\" % ast[0])"},{"line_number":97,"context_line":""},{"line_number":98,"context_line":"    op \u003d op_methods[ast[0]]"},{"line_number":99,"context_line":"    return op(cmp_value, *ast[1:])"}],"source_content_type":"text/x-python","patch_set":3,"id":"1aa78d24_f6aa3b18","line":97,"in_reply_to":"1aa78d24_e52a878f","updated":"2016-07-11 15:59:43.000000000","message":"Taking your second point first - there aren\u0027t any tests asserting that you should be able to pass in ints and I\u0027m loathe to expand the scope of this unnecessarily. It\u0027s not hard to str() an int if needed.\n\nI did consider trying to fix the type. If a single thing is passed with \u003call-in\u003e, we could str() it and treat it as a single-value list. The trouble is that Nova\u0027s usage - passing str([...]) - will then fail to match rather than raising an exception, which could make their migration more difficult (in principle - there are only 2 spots afaik). I think we should be strict for now and maybe after Nova+Ironic have migrated, we can be a bit more relaxed.\n\nI\u0027d rather not try eval\u0027ing cmp_value or even using the slightly safer, um, is it Python AST you can use? Much easier for Nova to fix their usage, then we can have some sanity.","commit_id":"58143fdcb79e641dd23a4dc78109dcedcf3530fd"},{"author":{"_account_id":6618,"name":"Ruby Loo","email":"opensrloo@gmail.com","username":"rloo"},"change_message_id":"d5fb201d826bec722966e639cc5ba8295724dab4","unresolved":false,"context_lines":[{"line_number":94,"context_line":"                            \" iterable\" % ast[0])"},{"line_number":95,"context_line":"    elif not is_str:"},{"line_number":96,"context_line":"        raise TypeError(\"%s expressions require value to be a string\" % ast[0])"},{"line_number":97,"context_line":""},{"line_number":98,"context_line":"    op \u003d op_methods[ast[0]]"},{"line_number":99,"context_line":"    return op(cmp_value, *ast[1:])"}],"source_content_type":"text/x-python","patch_set":3,"id":"1aa78d24_b53d402d","line":97,"in_reply_to":"1aa78d24_f6aa3b18","updated":"2016-07-11 19:25:04.000000000","message":"It seems odd/inconsistent to me, to have a method like this insist that \u0027cmp_value\u0027 is a string ONLY if it is not an \u0027\u003call-in\u003e\u0027 operator, and a list if it is \u0027\u003call-in\u003e.\n\nIf you do this, I think it is worth putting a comment/docstring to describe what this method expects.\n\nI am not totally sure how nova can fix their usage to call this. It seems like they\u0027d have to:\n1. *only* do str() before calling this, if the operator is not \u0027\u003call-in\u003e\u0027 (and doing this if seems wrong if that\u0027s sort of what this method is meant to deal with\u003e\n2. not do str() before calling this, but that won\u0027t work with this patch.\n3. *only* do str() IF the argument is not a list?","commit_id":"58143fdcb79e641dd23a4dc78109dcedcf3530fd"}],"oslo_utils/tests/test_specs_matcher.py":[{"author":{"_account_id":9796,"name":"ChangBo Guo","email":"glongwave@gmail.com","username":"gcb"},"change_message_id":"958ad8791b214d82d1264f23c6c5777e092a4b5d","unresolved":false,"context_lines":[{"line_number":262,"context_line":"            value\u003d\u0027aes\u0027,"},{"line_number":263,"context_line":"            req\u003d\u0027\u003call-in\u003e aes\u0027)"},{"line_number":264,"context_line":""},{"line_number":265,"context_line":"    def test_specs_errors_str_list_with_op_allin(self):"},{"line_number":266,"context_line":"        self.assertRaises("},{"line_number":267,"context_line":"            TypeError,"},{"line_number":268,"context_line":"            specs_matcher.match,"},{"line_number":269,"context_line":"            value\u003dstr([\u0027aes\u0027, \u0027mmx\u0027, \u0027aux\u0027]),"},{"line_number":270,"context_line":"            req\u003d\u0027\u003call-in\u003e aes\u0027)"}],"source_content_type":"text/x-python","patch_set":3,"id":"1aa78d24_44828b95","line":270,"range":{"start_line":265,"start_character":0,"end_line":270,"end_character":31},"updated":"2016-07-11 11:50:17.000000000","message":"I\u0027m afraid this will break Nova, need confirm this \n\n\u003call-in\u003e was first added in https://github.com/openstack/nova/commit/d6304a0a122d2c0ecf82fb29dcf6b2e7c767aa80,\n\nIt expects value is a string, not a list,  see https://github.com/openstack/nova/blob/master/nova/scheduler/filters/compute_capabilities_filter.py#L87","commit_id":"58143fdcb79e641dd23a4dc78109dcedcf3530fd"},{"author":{"_account_id":8688,"name":"Alexis Lee","email":"openstack@lxsli.co.uk","username":"lxsli"},"change_message_id":"e290eedba00c518e8b6a527e38e6fda0dc191a43","unresolved":false,"context_lines":[{"line_number":262,"context_line":"            value\u003d\u0027aes\u0027,"},{"line_number":263,"context_line":"            req\u003d\u0027\u003call-in\u003e aes\u0027)"},{"line_number":264,"context_line":""},{"line_number":265,"context_line":"    def test_specs_errors_str_list_with_op_allin(self):"},{"line_number":266,"context_line":"        self.assertRaises("},{"line_number":267,"context_line":"            TypeError,"},{"line_number":268,"context_line":"            specs_matcher.match,"},{"line_number":269,"context_line":"            value\u003dstr([\u0027aes\u0027, \u0027mmx\u0027, \u0027aux\u0027]),"},{"line_number":270,"context_line":"            req\u003d\u0027\u003call-in\u003e aes\u0027)"}],"source_content_type":"text/x-python","patch_set":3,"id":"1aa78d24_955adb28","line":270,"range":{"start_line":265,"start_character":0,"end_line":270,"end_character":31},"in_reply_to":"1aa78d24_44828b95","updated":"2016-07-11 12:26:47.000000000","message":"Yes, it will. Nova will need to change their usage when they switch to the oslo_utils version of the matcher. This is better than us accepting broken code to maintain bug-for-bug.","commit_id":"58143fdcb79e641dd23a4dc78109dcedcf3530fd"}]}