feat: Add support for handling PR updates in asana task attachments#212
feat: Add support for handling PR updates in asana task attachments#212savokr-asana wants to merge 2 commits intomasterfrom
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
savokr-asana
changed the title
vn6
left a comment
vn6
left a comment
There was a problem hiding this comment.
functionality looks good! just a couple comments to clean up the logic here
src/asana/helpers.py
Outdated
| ] | ||
|
|
||
| # Update the attachments mapping in DynamoDB | ||
| dynamodb_client.update_attachments_for_github_node(github_node_id, new_attachments) |
There was a problem hiding this comment.
this can be simplified to just use the current_asset_ids instead of creating a new list for new_attachments (though would need to fix types)
| dynamodb_client.update_attachments_for_github_node(github_node_id, new_attachments) | |
| dynamodb_client.update_attachments_for_github_node(github_node_id, current_asset_ids) |
and change the for loop above to just go through assets_to_create
There was a problem hiding this comment.
I am not sure I got your point but I feel like current implementation is good enough. We can't change the types for update_attachments function since it need the full attachments information, not just asset_ids. I've renamed some of the variables to make things more clear but don't really see that it can be simplified further. Also, attachments count should be really low, no more than around 10, so I don't think it will have any performance impact either.
savokr-asana
force-pushed
the
saveliinovikov-update-asana-attachments
branch
from
7ef0ccc to
9bcaf06
Compare
savokr-asana
force-pushed
the
saveliinovikov-update-asana-attachments
branch
from
9bcaf06 to
741b957
Compare
2 participants
Description
This PR introduces support for handling updates of PR description/comments and updating corresponding attachments in asana task.
Test
Added unit tests are passing, added tests for all new functions.
Pull Request synchronized with Asana task
Pull Request: #212