-
Notifications
You must be signed in to change notification settings - Fork 301
Create a new Delivery
struct that includes Timestamp
#555
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
Conversation
**Note:** This is an API breaking change. Currently the `Timestamp` value is being dropped as part of the conversion from `BorrowedMessage` to `OwnedDeliveryResult`, however this value is updated once the message has been published and callers should have access to the updated value. As far as I can tell, this is the **only** other value that is updated as part of publishing. An alternative to expanding the tuple would be to replace it with a struct. This is a bit more disruptive now, but would making adding values in the future a backwards compatible change. So it depends how likely it is that there will be further new values. This could also be done in a way that isn't a breaking change, but would involve duplicating functions and copying some types, i.e. `DeliveryFuture`. It doesn't seem like that level of complexity is justified given the pre 1.0 status.
Any chance someone could take a look at this? I would like get our production systems off my fork if at all possible. |
@davidblewett When you get a moment, could you review this PR? We'd love to stop maintaining a fork for this small change. ❤️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer a dedicated struct, for the reason described in the comment. If we're going to break the API, I'd prefer to do it once and be able to adjust it in the future without breaking users.
Thanks, @davidblewett! Good to know. We'll get back to you soon with that change. |
Just to set expectations, we'll most likely deliver this change next month after the holidays. We want to make sure to give it a thorough review and testing on our end before submitting it upstream. Happy Holidays! 😄 |
I have switched this out. Let me know if there is anything else you would like me to change. |
Timestamp
to the OwnedDeliveryResult
tuple.Delivery
struct that includes Timestamp
@davidblewett I hope you had some fantastic winter holidays! How is this PR looking to you? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last request I promise: please add a changelog entry documenting the API break for the OwnedDeliveryResult
type.
@davidblewett changelog updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Note: This is an API breaking change.
Currently the
Timestamp
value is being dropped as part of the conversion fromBorrowedMessage
toOwnedDeliveryResult
, however this value is updated once the message has been published and callers should have access to the updated value. As far as I can tell, this is the only other value that is updated as part of publishing.The new struct is returned as part of the
OwnedDeliveryResult
.An alternative to expanding the tuple would be to replace it with a struct. This is a bit more disruptive now, but would make adding values in the future a backwards compatible change. So it depends how likely it is that there will be further new values.This could also be done in a way that isn't a breaking change, but would involve duplicating functions and copying some types, i.e.
DeliveryFuture
. It doesn't seem like that level of complexity is justified given the pre 1.0 status.