Conversation
lbergelson
left a comment
lbergelson
left a comment
There was a problem hiding this comment.
@SHuang-Broad Looks fine. Minor comments. At some point we might want to move it to utils if people are going to call into it a lot.
| ret_val = updated_ret_val | ||
|
|
||
| # Display status to user: | ||
| line_string = requested_status_json |
There was a problem hiding this comment.
why rename this here? Lets just print the requested_status_json after replacement.
There was a problem hiding this comment.
+1
Plus It isn't good practice to use type suffix when naming variables
There was a problem hiding this comment.
Push back a little: I didn't change this as it was in the original code. I used pycharm to refactor out a function.
Regardless, I'll update this PR with the requested changes, and move this to utils.
| ret_val = 1 | ||
| log.display_logo(io_utils.dead_turtle) | ||
| elif workflow_status == "Running": | ||
| elif workflow_status == cromshellconfig.WorkflowStatuses.Running.value[0]: |
There was a problem hiding this comment.
This is weird. I feel like there must be a better solution but we don't have to find it right now.
There was a problem hiding this comment.
can you elaborate? I'm hoping to do it correctly in this PR.
There was a problem hiding this comment.
It feels like there should be a less awkward way of matching against the enum values. We shouldn't have to decompose the list manually. Maybe we should overload the equality option in the enum to check all the list values so we don't have to do this awkward thing.
|
|
||
| # Display status to user: | ||
| line_string = requested_status_json | ||
| print(line_string.replace(",", ",\n")) |
There was a problem hiding this comment.
Its fine as is but would be nice to use pretty_print_json()
| return ret_val | ||
|
|
||
|
|
||
| def query_status(config, workflow_id: str) -> (str, str, int): |
There was a problem hiding this comment.
Not now but later, would be good to add unit tests
There was a problem hiding this comment.
agree, I'll add unit test in this PR.
| return ret_val | ||
|
|
||
|
|
||
| def query_status(config, workflow_id: str) -> (str, str, int): |
There was a problem hiding this comment.
agree, I'll add unit test in this PR.
| ret_val = 1 | ||
| log.display_logo(io_utils.dead_turtle) | ||
| elif workflow_status == "Running": | ||
| elif workflow_status == cromshellconfig.WorkflowStatuses.Running.value[0]: |
There was a problem hiding this comment.
can you elaborate? I'm hoping to do it correctly in this PR.
3 participants
So that other commands can reuse its logic (notably,
notify).