)]}'
{"mistral/db/v2/sqlalchemy/api.py":[{"author":{"_account_id":8731,"name":"Renat Akhmerov","email":"renat.akhmerov@gmail.com","username":"rakhmerov"},"change_message_id":"77d3c08df0e5f083d2328d7e548b41fce9a5d41c","unresolved":false,"context_lines":[{"line_number":344,"context_line":"        )"},{"line_number":345,"context_line":"    if _check_associate_cron_trigger(name):"},{"line_number":346,"context_line":"        raise exc.DBException("},{"line_number":347,"context_line":"            \"Can\u0027t delete workflow that has triggers [workflow_name\u003d%s]\" % name"},{"line_number":348,"context_line":"        )"},{"line_number":349,"context_line":""},{"line_number":350,"context_line":"    session.delete(wf_def)"}],"source_content_type":"text/x-python","patch_set":2,"id":"7a2fa921_3207b519","line":347,"updated":"2015-10-08 08:57:03.000000000","message":"Instead of just saying that there are triggers I would also let user know names of those triggers. IMO, method _check_associated_cron_trigger() should be refactored to return a list of triggers so that we could print them out.","commit_id":"a5792818cc9f759362624479ce9681b847af8a53"},{"author":{"_account_id":6732,"name":"Lingxian Kong","email":"anlin.kong@gmail.com","username":"kong"},"change_message_id":"17e265e27f560c3c00df620e082ed5f840352899","unresolved":false,"context_lines":[{"line_number":344,"context_line":"        )"},{"line_number":345,"context_line":"    if _check_associate_cron_trigger(name):"},{"line_number":346,"context_line":"        raise exc.DBException("},{"line_number":347,"context_line":"            \"Can\u0027t delete workflow that has triggers [workflow_name\u003d%s]\" % name"},{"line_number":348,"context_line":"        )"},{"line_number":349,"context_line":""},{"line_number":350,"context_line":"    session.delete(wf_def)"}],"source_content_type":"text/x-python","patch_set":2,"id":"7a2fa921_9289e977","line":347,"in_reply_to":"7a2fa921_3207b519","updated":"2015-10-08 09:01:31.000000000","message":"yes, agree with Renat. Error message should tell useful information to users.","commit_id":"a5792818cc9f759362624479ce9681b847af8a53"},{"author":{"_account_id":8731,"name":"Renat Akhmerov","email":"renat.akhmerov@gmail.com","username":"rakhmerov"},"change_message_id":"77d3c08df0e5f083d2328d7e548b41fce9a5d41c","unresolved":false,"context_lines":[{"line_number":350,"context_line":"    session.delete(wf_def)"},{"line_number":351,"context_line":""},{"line_number":352,"context_line":""},{"line_number":353,"context_line":"def _check_associate_cron_trigger(name):"},{"line_number":354,"context_line":"    if _secure_query(models.CronTrigger).filter_by(workflow_name\u003dname).first():"},{"line_number":355,"context_line":"        return True"},{"line_number":356,"context_line":"    else:"}],"source_content_type":"text/x-python","patch_set":2,"id":"7a2fa921_77975fec","line":353,"updated":"2015-10-08 08:57:03.000000000","message":"Two suggestions:\n\n1. \u0027name\u0027 -\u003e \u0027wf_name\u0027, otherwise it seems like this method takes a trigger name rather than a workflow name.\n\n2. \u0027associate\u0027 -\u003e \u0027associated\u0027","commit_id":"a5792818cc9f759362624479ce9681b847af8a53"},{"author":{"_account_id":8731,"name":"Renat Akhmerov","email":"renat.akhmerov@gmail.com","username":"rakhmerov"},"change_message_id":"77d3c08df0e5f083d2328d7e548b41fce9a5d41c","unresolved":false,"context_lines":[{"line_number":351,"context_line":""},{"line_number":352,"context_line":""},{"line_number":353,"context_line":"def _check_associate_cron_trigger(name):"},{"line_number":354,"context_line":"    if _secure_query(models.CronTrigger).filter_by(workflow_name\u003dname).first():"},{"line_number":355,"context_line":"        return True"},{"line_number":356,"context_line":"    else:"},{"line_number":357,"context_line":"        return False"}],"source_content_type":"text/x-python","patch_set":2,"id":"7a2fa921_7733df14","line":354,"updated":"2015-10-08 08:57:03.000000000","message":"It can be just:\n\nreturn bool(_secure_query(models.CronTrigger)...)","commit_id":"a5792818cc9f759362624479ce9681b847af8a53"},{"author":{"_account_id":8731,"name":"Renat Akhmerov","email":"renat.akhmerov@gmail.com","username":"rakhmerov"},"change_message_id":"f98e2be7b24a20029709c271402d8e200a4a8e86","unresolved":false,"context_lines":[{"line_number":343,"context_line":"            \"Workflow not found [workflow_name\u003d%s]\" % name"},{"line_number":344,"context_line":"        )"},{"line_number":345,"context_line":""},{"line_number":346,"context_line":"    cron_triggers \u003d _get_associated_cron_triggers_list(name)"},{"line_number":347,"context_line":""},{"line_number":348,"context_line":"    if cron_triggers:"},{"line_number":349,"context_line":"        raise exc.DBException("}],"source_content_type":"text/x-python","patch_set":3,"id":"7a2fa921_1dfcd76e","line":346,"updated":"2015-10-09 07:28:47.000000000","message":"\"_list\" is redundant in this name. When you use plural form it already tells us it\u0027s a collection.","commit_id":"4cb7a9f203c2b3a99785b30685d001de693a0936"},{"author":{"_account_id":8731,"name":"Renat Akhmerov","email":"renat.akhmerov@gmail.com","username":"rakhmerov"},"change_message_id":"f98e2be7b24a20029709c271402d8e200a4a8e86","unresolved":false,"context_lines":[{"line_number":355,"context_line":""},{"line_number":356,"context_line":""},{"line_number":357,"context_line":"def _get_associated_cron_triggers_list(wf_name):"},{"line_number":358,"context_line":"    query \u003d _secure_query("},{"line_number":359,"context_line":"        models.CronTrigger,"},{"line_number":360,"context_line":"        models.CronTrigger.name"},{"line_number":361,"context_line":"    ).filter_by(workflow_name\u003dwf_name).all()"}],"source_content_type":"text/x-python","patch_set":3,"id":"7a2fa921_fd71db0c","line":358,"updated":"2015-10-09 07:28:47.000000000","message":"\"query\" is a wrong name. Query is what you get using _secure_query() method. But you already get a collection of cron triggers using this query with this call.\n\nRename \"query\" to \"cron_triggers\" or \"cron_trigger_names\".\n\nBtw, word \"cron\" can be safely omitted since it\u0027s clear from the context (method name) that we deal with cron triggers here. I suggest we use just \"trigger\".","commit_id":"4cb7a9f203c2b3a99785b30685d001de693a0936"},{"author":{"_account_id":8731,"name":"Renat Akhmerov","email":"renat.akhmerov@gmail.com","username":"rakhmerov"},"change_message_id":"f98e2be7b24a20029709c271402d8e200a4a8e86","unresolved":false,"context_lines":[{"line_number":360,"context_line":"        models.CronTrigger.name"},{"line_number":361,"context_line":"    ).filter_by(workflow_name\u003dwf_name).all()"},{"line_number":362,"context_line":""},{"line_number":363,"context_line":"    return [cron_triggers[0] for cron_triggers in query]"},{"line_number":364,"context_line":""},{"line_number":365,"context_line":""},{"line_number":366,"context_line":"@b.session_aware()"}],"source_content_type":"text/x-python","patch_set":3,"id":"7a2fa921_9d15479d","line":363,"updated":"2015-10-09 07:28:47.000000000","message":"1. No need to use such a long name for local variables. Instead of \"cron_triggers\" it could be just \"trig\" or \"t\".\n\n2. \"cron_triggers\" semantically is a wrong name because it\u0027s a plural form whereas you this is variable is not a collection, it just contains a single trigger name.","commit_id":"4cb7a9f203c2b3a99785b30685d001de693a0936"}]}