)]}'
{"oslo_db/sqlalchemy/enginefacade.py":[{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"6d87bde8f6379be895f153b1d783c31af9e3b51e","unresolved":false,"context_lines":[{"line_number":1007,"context_line":"        @functools.wraps(fn)"},{"line_number":1008,"context_line":"        def wrapper(*args, **kwargs):"},{"line_number":1009,"context_line":"            context_candidate \u003d kwargs.get(\u0027context\u0027, None)"},{"line_number":1010,"context_line":"            if _is_transaction_context_provider(context_candidate):"},{"line_number":1011,"context_line":"                context \u003d context_candidate"},{"line_number":1012,"context_line":"            else:"},{"line_number":1013,"context_line":"                context \u003d args[context_index]"}],"source_content_type":"text/x-python","patch_set":2,"id":"5fc1f717_18edc4b6","line":1010,"updated":"2019-04-03 14:20:33.000000000","message":"what\u0027s the use case where a function that\u0027s decorated with @enginefacade, receives a keyword argument that\u0027s named \"context\", and that object is *not* the one that is used for the enginefacade?   I wonder if this should just raise an error if \"context\u003dxyz\" is present and it does not have the attribute.    Or warn.   It should at least warn if context is present and isn\u0027t the transaction context?","commit_id":"e070b6ae63a164ffcd216d4dc1a9a3a92ccaac38"},{"author":{"_account_id":28714,"name":"Adrian Chiris","email":"adrianc@nvidia.com","username":"adrianc"},"change_message_id":"9104f031ab23aa8fb6942bd095ff05f554544e6e","unresolved":false,"context_lines":[{"line_number":1007,"context_line":"        @functools.wraps(fn)"},{"line_number":1008,"context_line":"        def wrapper(*args, **kwargs):"},{"line_number":1009,"context_line":"            context_candidate \u003d kwargs.get(\u0027context\u0027, None)"},{"line_number":1010,"context_line":"            if _is_transaction_context_provider(context_candidate):"},{"line_number":1011,"context_line":"                context \u003d context_candidate"},{"line_number":1012,"context_line":"            else:"},{"line_number":1013,"context_line":"                context \u003d args[context_index]"}],"source_content_type":"text/x-python","patch_set":2,"id":"5fc1f717_639d22ba","line":1010,"in_reply_to":"5fc1f717_18edc4b6","updated":"2019-04-04 10:48:03.000000000","message":"yes, the idea was to support context as kw arg if it has a transaction context else fall back to the original flow of taking the positional argument as the context.\n\nI didnt want to limit the user as the following should still work:\n\n@enginefacade.writer\ndef some_db_method(db_ctx, context, other_param)\n    ....\n\ncalling it as follows:\nsome_db_method(database_ctx, context\u003dsome_other_context, other_param\u003dparams)","commit_id":"e070b6ae63a164ffcd216d4dc1a9a3a92ccaac38"},{"author":{"_account_id":28714,"name":"Adrian Chiris","email":"adrianc@nvidia.com","username":"adrianc"},"change_message_id":"9104f031ab23aa8fb6942bd095ff05f554544e6e","unresolved":false,"context_lines":[{"line_number":1011,"context_line":"                context \u003d context_candidate"},{"line_number":1012,"context_line":"            else:"},{"line_number":1013,"context_line":"                context \u003d args[context_index]"},{"line_number":1014,"context_line":""},{"line_number":1015,"context_line":"            with self._transaction_scope(context):"},{"line_number":1016,"context_line":"                return fn(*args, **kwargs)"},{"line_number":1017,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"5fc1f717_43d10657","line":1014,"updated":"2019-04-04 10:48:03.000000000","message":"Maybe add check here to make sure the selected context (kw vs pos) is a transaction context provider ? so we cover both cases. If the selected context is not a transaction context provider i think we should raise an error.\n\nSounds good ?","commit_id":"e070b6ae63a164ffcd216d4dc1a9a3a92ccaac38"},{"author":{"_account_id":28714,"name":"Adrian Chiris","email":"adrianc@nvidia.com","username":"adrianc"},"change_message_id":"0358d86db8586ed1b932b8a9f59aa4a9e640e0ff","unresolved":false,"context_lines":[{"line_number":1011,"context_line":"                context \u003d context_candidate"},{"line_number":1012,"context_line":"            else:"},{"line_number":1013,"context_line":"                context \u003d args[context_index]"},{"line_number":1014,"context_line":""},{"line_number":1015,"context_line":"            with self._transaction_scope(context):"},{"line_number":1016,"context_line":"                return fn(*args, **kwargs)"},{"line_number":1017,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"5fc1f717_68de5adb","line":1014,"in_reply_to":"5fc1f717_1294a9e7","updated":"2019-04-04 14:26:31.000000000","message":"I agree its an \"exotic\" case.\nthe plus for adding a check here is that we will fail with an informative error in case the selected context is not a transaction context (not just in the keyword use-case).\n\nwill re-spin it soon","commit_id":"e070b6ae63a164ffcd216d4dc1a9a3a92ccaac38"},{"author":{"_account_id":28714,"name":"Adrian Chiris","email":"adrianc@nvidia.com","username":"adrianc"},"change_message_id":"35c3327d81e0b3a3ea89fbea3ad0da3e641d60ea","unresolved":false,"context_lines":[{"line_number":1011,"context_line":"                context \u003d context_candidate"},{"line_number":1012,"context_line":"            else:"},{"line_number":1013,"context_line":"                context \u003d args[context_index]"},{"line_number":1014,"context_line":""},{"line_number":1015,"context_line":"            with self._transaction_scope(context):"},{"line_number":1016,"context_line":"                return fn(*args, **kwargs)"},{"line_number":1017,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"5fc1f717_5fef6db0","line":1014,"in_reply_to":"5fc1f717_2075bdfa","updated":"2019-04-07 15:58:04.000000000","message":"Much cleaner, thanks!\n\nwill upload a PS soon","commit_id":"e070b6ae63a164ffcd216d4dc1a9a3a92ccaac38"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"3a0a0881783611c252be54709c039c72ee1b6335","unresolved":false,"context_lines":[{"line_number":1011,"context_line":"                context \u003d context_candidate"},{"line_number":1012,"context_line":"            else:"},{"line_number":1013,"context_line":"                context \u003d args[context_index]"},{"line_number":1014,"context_line":""},{"line_number":1015,"context_line":"            with self._transaction_scope(context):"},{"line_number":1016,"context_line":"                return fn(*args, **kwargs)"},{"line_number":1017,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"5fc1f717_1294a9e7","line":1014,"in_reply_to":"5fc1f717_43d10657","updated":"2019-04-04 12:53:53.000000000","message":"that is a good idea, but I still remain uncomfortable that someone has converted their application to use enginefacade, has added the decorators, yet is using a keyword named \"context\" for other reasons.   but if someone is doing that, then they\u0027re doing it, so I guess we should do it that way.","commit_id":"e070b6ae63a164ffcd216d4dc1a9a3a92ccaac38"},{"author":{"_account_id":28714,"name":"Adrian Chiris","email":"adrianc@nvidia.com","username":"adrianc"},"change_message_id":"aaea869cf595a7c0deb2a47011dd301c74e6181f","unresolved":false,"context_lines":[{"line_number":1011,"context_line":"                context \u003d context_candidate"},{"line_number":1012,"context_line":"            else:"},{"line_number":1013,"context_line":"                context \u003d args[context_index]"},{"line_number":1014,"context_line":""},{"line_number":1015,"context_line":"            with self._transaction_scope(context):"},{"line_number":1016,"context_line":"                return fn(*args, **kwargs)"},{"line_number":1017,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"5fc1f717_d7104a03","line":1014,"in_reply_to":"5fc1f717_68de5adb","updated":"2019-04-04 18:01:27.000000000","message":"OK, looking again on _TransactionContextManager._transaction_scope() which calls _TransactionContextManager._transaction_scope_by_thread() i see that if the \"context\" is not the one used by enginefacade Then a _TransactionContextTLocal() is injected and used [1]\n\nI may be getting things wrong due to the late hour but if that is the case then i think its best to go with:\n\nif user provided a context kwarg then simply assume its the enginefacade context else take context from positional arguments.(basically remove the elaborate check)\n\nWhat do you think ?\n\n\n[1]https://github.com/openstack/oslo.db/blob/57333e902a3747715b2f13ea63477cc4aedc2129/oslo_db/sqlalchemy/enginefacade.py#L1124","commit_id":"e070b6ae63a164ffcd216d4dc1a9a3a92ccaac38"},{"author":{"_account_id":11816,"name":"mike_mp@zzzcomputing.com","display_name":"Mike Bayer","email":"mike_mp@zzzcomputing.com","username":"zzzeek","status":"Red Hat"},"change_message_id":"2a0289a5e2a43c2168b8c88a3ba611a6f338335b","unresolved":false,"context_lines":[{"line_number":1011,"context_line":"                context \u003d context_candidate"},{"line_number":1012,"context_line":"            else:"},{"line_number":1013,"context_line":"                context \u003d args[context_index]"},{"line_number":1014,"context_line":""},{"line_number":1015,"context_line":"            with self._transaction_scope(context):"},{"line_number":1016,"context_line":"                return fn(*args, **kwargs)"},{"line_number":1017,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"5fc1f717_2075bdfa","line":1014,"in_reply_to":"5fc1f717_d7104a03","updated":"2019-04-05 02:57:34.000000000","message":"OK a lot of these weird cases were when I worked with Matthew Booth on this and we were really trying to anticipate a lot of scenarios at that time.  We could still test the positional argument for the attribute maybe... but I don\u0027t like doing the test anyway.   \n\nI just noticed we are getting the argspec of the function anyway right above.   Why don\u0027t we just get the name of the context argument right there?  it\u0027s argspec.args[1].  That\u0027s the kw name.  we look for that in **kw if it\u0027s there and we\u0027re done.   this is already how context_index works.  Sorry this is what I should have remembered earlier.","commit_id":"e070b6ae63a164ffcd216d4dc1a9a3a92ccaac38"}]}