Skip to content
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

PubSub: Message publish_time return type unexpected #5598

Closed
alexcwatt opened this issue Jul 12, 2018 · 5 comments
Closed

PubSub: Message publish_time return type unexpected #5598

alexcwatt opened this issue Jul 12, 2018 · 5 comments
Assignees
Labels
api: pubsub Issues related to the Pub/Sub API. priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@alexcwatt
Copy link

I am working on a PubSub project and was a bit confused about the Message publish_time. I expected to get datetime per the docstring for the method. Instead I got a Timestamp that apparently comes from protobuf types. I found there is even a unit test that verifies that publish_time returns a Timestamp.

Can we return datetime? Or should the docs just be updated to explain what is actually being returned? I'd prefer the former but wanted to ask before submitting a PR, given the inconsistency.

@tseaver tseaver added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. api: pubsub Issues related to the Pub/Sub API. priority: p2 Moderately-important priority. Fix may not be included in next release. priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. and removed priority: p2 Moderately-important priority. Fix may not be included in next release. labels Jul 12, 2018
@tseaver
Copy link
Contributor

tseaver commented Jul 12, 2018

@alexcwatt Thanks for the report! I believe that updating the implementation / test to convert the integer timestamp to a date time is best.

@tseaver
Copy link
Contributor

tseaver commented Jul 12, 2018

@theacodes I marked this as triaged for GA because the return type of the publish_time method property needs to change to match the docstring: ISTM that is a "surface" change which we might want to fix before GA.

@JustinBeckwith JustinBeckwith added the triage me I really want to be triaged. label Jul 13, 2018
@JustinBeckwith JustinBeckwith removed the triage me I really want to be triaged. label Jul 13, 2018
@tseaver
Copy link
Contributor

tseaver commented Jul 16, 2018

@theacodes Hmm, apparently I misunderstood the triaged for GA label: I think this issue should be fixed before GA.

@tseaver tseaver assigned tseaver and unassigned theacodes Jul 16, 2018
@tseaver
Copy link
Contributor

tseaver commented Jul 16, 2018

@alexcwatt If you don't already have a PR in hand, I'm going to go ahead this week, so that we can get the change in place before the upcoming GA release.

tseaver added a commit that referenced this issue Jul 16, 2018
Rather than a 'google.protobuf.timestamp_pb2.Timestamp' instance.

Closes #5598.
@alexcwatt
Copy link
Author

@tseaver Awesome, thanks!

tseaver added a commit that referenced this issue Jul 17, 2018
Rather than a 'google.protobuf.timestamp_pb2.Timestamp' instance.

Closes #5598.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the Pub/Sub API. priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

No branches or pull requests

4 participants