Conversation
giograno
added
area: persistence
Test Results (amd64) - Integration, Bootstrap 5 files 5 suites 2h 39m 55s ⏱️ Results for commit 70f7bfd. ♻️ This comment has been updated with latest results. |
LocalStack Community integration with Pro 2 files 2 suites 1h 59m 8s ⏱️ Results for commit 70f7bfd. ♻️ This comment has been updated with latest results. |
giograno
requested review from
baermat,
dominikschubert,
gregfurman,
simonrw and
thrau
as code owners
giograno
requested review from
bentsku
and removed request for
dominikschubert,
gregfurman and
simonrw
baermat
left a comment
baermat
left a comment
There was a problem hiding this comment.
I am honestly surprised that all our tests still pass with the removal or addition of certain default values, so good job cleaning those up! 😄 Not counting those that were simply moved of course.
I have 2 questions, once those are answered (not necessarily with code change), we are good to go from my side.
| message_deduplication_id: str | ||
| message_group_id: str |
There was a problem hiding this comment.
Where was the problem with message_deduplocation_id and message_group_id? 🙂
There was a problem hiding this comment.
If you look at the constructor, they are provided as parameter but an instance variable is never assigned. They are used to populate the attributes dict.
| class StandardQueue(SqsQueue): | ||
| visible: InterruptiblePriorityQueue[SqsMessage] |
There was a problem hiding this comment.
This seems like a real loss of type information, is there no way of retaining it?
There was a problem hiding this comment.
InterruptiblePriorityQueue is not declared as a generic class. Therefore, the parametrization was not really correct.
It is only used in SQS with SqsMessage to the best of my knowledge. We could just remove the parametrization as I did, or declare the class generic (something like class InterruptiblePriorityQueue[T](PriorityQueue, InterruptibleQueue): .... I do not have big preferences.
There was a problem hiding this comment.
let's make InterruptiblePriorityQueue generic!
3 participants
Motivation
With the push towards Avro-based serialization, we have to improve the type annotations for the stores.
Changes