Create review subtasks for requested reviewers #110
Conversation
gregeinfrank
left a comment
gregeinfrank
left a comment
There was a problem hiding this comment.
Awesome start - thanks so much for picking this up. A couple comments, but directionally this looks good. Of course before getting ready for real review we'll need to add tests.
I'm also a bit worried about how this rollout will work at Asana, and anticipate lots of complaints from engineers who are going to get duplicate tasks. That's probably OK as long as we get ahead of it and communicate it in advance and get people to change their behavior to use CC correctly and not add everyone as reviewers no matter what.
Also good to loop in the code review AOR holder before rolling out, I think they were involved in the original discussion around this but this is a big enough process change that there should be a coordinated rollout. Thanks!
src/asana/client.py
Outdated
| return response["gid"] | ||
|
|
||
| def create_subtask( | ||
| self, parent_task_id: str, reviewer: str, task_name: str, task_description, |
There was a problem hiding this comment.
nit: I'd just call this assignee since this is a wrapper around the Asana API and doesn't necessarily need to know the context of "reviewer" - just to format and forward the request to the API
| self, parent_task_id: str, reviewer: str, task_name: str, task_description, | |
| self, parent_task_id: str, assignee: str, task_name: str, task_description, |
src/asana/client.py
Outdated
| return AsanaClient.singleton().update_task(task_id, fields) | ||
|
|
||
|
|
||
| def is_task_completed(task_id) -> bool: |
There was a problem hiding this comment.
I think the pattern established here is to keep the naming consistent
| def is_task_completed(task_id) -> bool: | |
| def get_task_completed_status(task_id) -> bool: |
src/asana/controller.py
Outdated
| maybe_complete_tasks_on_merge(pull_request) | ||
|
|
||
|
|
||
| def create_review_subtask(pull_request: PullRequest, parent_task_id: str, reviewer_handle: str): |
There was a problem hiding this comment.
| def create_review_subtask(pull_request: PullRequest, parent_task_id: str, reviewer_handle: str): | |
| def create_review_subtask(pull_request: PullRequest, parent_task_id: str, reviewer_handle: str) -> str: |
src/asana/controller.py
Outdated
| ) | ||
|
|
||
|
|
||
| def update_subtask(pull_request: PullRequest, subtask_id): |
There was a problem hiding this comment.
The naming here is a bit confusing, perhaps just reopen_subtask_if_completed would be more accurate?
| key is formed by concatenating the two GitHub node ids using a "-" separator. | ||
| """ | ||
| dynamo_db_key = _get_dynamodb_key_from_two_github_nodes(gh_node_id_a, gh_node_id_b) | ||
| print(f"Inserting key {dynamo_db_key} to DynamoDb.") |
There was a problem hiding this comment.
| print(f"Inserting key {dynamo_db_key} to DynamoDb.") | |
| logger.info(f"Inserting key {dynamo_db_key} to DynamoDb.") |
| ) | ||
|
|
||
| def _get_dynamodb_key_from_two_github_nodes(gh_node_id_a: str, gh_node_id_b: str) -> str: | ||
| return f"{gh_node_id_a}-{gh_node_id_b}" |
There was a problem hiding this comment.
Perhaps this function should sort the two node ids, so we don't have to remember which order to pass them in and reduce bugs around unexpected orders
| ) | ||
|
|
||
|
|
||
| def update_subtasks(pull_request: PullRequest, task_id: str) -> None: |
There was a problem hiding this comment.
I think it'd be more efficient to do this and upsert_subtasks in one loop rather than two separate functions. Mostly because it would cut the calls to dynamodb in half and updating wouldn't be necessary for newly created subtasks
|
It'd be so nice to have something like this! |
This is still WIP.
Pull Request synchronized with Asana task