Skip to content

Tech debt: BasePartialBatchProcessor response() and process_partial_response() should have the same response typing #3020

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
1 of 2 tasks
adriantomas opened this issue Aug 28, 2023 · 3 comments · Fixed by #3023
Assignees
Labels
batch Batch processing utility tech-debt Technical Debt tasks

Comments

@adriantomas
Copy link
Contributor

adriantomas commented Aug 28, 2023

Why is this needed?

This method https://github.com/aws-powertools/powertools-lambda-python/blob/develop/aws_lambda_powertools/utilities/batch/base.py#L256 and https://github.com/aws-powertools/powertools-lambda-python/blob/024c3f2ed8858643a2d303e5c3928aa58e168c7c/aws_lambda_powertools/utilities/batch/decorators.py#L128C16-L128C16

Should have the same response typing

class PartialItemFailureResponse(TypedDict):

So that users can easily guess that using one method or another produces the same output.

Which area does this relate to?

Static typing

Suggestion

I can submit a PR with the changes.

Acknowledgment

@adriantomas adriantomas added tech-debt Technical Debt tasks triage Pending triage from maintainers labels Aug 28, 2023
@boring-cyborg
Copy link

boring-cyborg bot commented Aug 28, 2023

Thanks for opening your first issue here! We'll come back to you as soon as we can.
In the meantime, check out the #python channel on our Powertools for AWS Lambda Discord: Invite link

@leandrodamascena
Copy link
Contributor

Hello @adriantomas! I totally agree that response() should have the same response type as process_partial_response(). When we refactored the Batch Process utility with the new process_partial_response() function, we created this type to make it easier for clients to access the properties of this response and we forget to annotate response() function, sorry for that 😞.

Please, send a PR to fix this and we can merge :)

Thanks a lot

@leandrodamascena leandrodamascena moved this from Triage to Pending customer in Powertools for AWS Lambda (Python) Aug 28, 2023
@leandrodamascena leandrodamascena added batch Batch processing utility and removed triage Pending triage from maintainers labels Aug 28, 2023
@leandrodamascena leandrodamascena linked a pull request Aug 29, 2023 that will close this issue
5 tasks
@github-project-automation github-project-automation bot moved this from Pending customer to Coming soon in Powertools for AWS Lambda (Python) Aug 29, 2023
@github-actions
Copy link
Contributor

⚠️COMMENT VISIBILITY WARNING⚠️

This issue is now closed. Please be mindful that future comments are hard for our team to see.

If you need more assistance, please either tag a team member or open a new issue that references this one.

If you wish to keep having a conversation with other community members under this issue feel free to do so.

@leandrodamascena leandrodamascena self-assigned this Aug 29, 2023
@leandrodamascena leandrodamascena moved this from Coming soon to Shipped in Powertools for AWS Lambda (Python) Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
batch Batch processing utility tech-debt Technical Debt tasks
Projects
Status: Shipped
Development

Successfully merging a pull request may close this issue.

2 participants