-
Notifications
You must be signed in to change notification settings - Fork 592
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
datastore: support streams with request.get #755
datastore: support streams with request.get #755
Conversation
// begin crazy talk I agree that streamRouter in its current state probably isn't the way to go. At first I was going to say that we should just make a separate method for streams since this api is a bit different than our typically streamRouter use case -- however, I think a similar problem may already exist (well, whether or not it's actually a problem is probably up for debate) Typically there's an option to set the maximum number of results to return, so what's the expectation when that max is set to 1 (not sure how likely this is to happen). As it is today, even though we only request 1 result, we still return an array -- this of course makes total sense, but it might offer slight inconvenience if the user is only expecting 1 result. What if we were to add logic that checked if the max was 1 and if so, returned the singular item instead of an array. After which maybe it would be possible to somehow set the max to 1 if we saw that this particular method only received a single key? ¯_(ツ)_/¯ |
This is super awesome to see!! Still want an issue for tracking or is the comment ref enough? This might be naive, but couldn't you continue to unpack the single-item array much like in the current implementation of Just seems a bit easier than handling the unpack at the streamRouter level - then you'd have to update other streamed methods to potentially expect a single value as well, I think. |
@davidrekow no worries about the issue :)
I think the road of least surprises is where we want to go. If we can document our method as always returning an array, nobody would expect to get an unboxed array under any conditions. Your suggestion is a pretty creative way to solve the problem, though, and it would work if we wanted our other APIs that accept I'm leaning towards just handling the stream logic in this method individually, and avoiding using streamRouter altogether. Maybe as we go on with our library, we'll find other similar places and we can start to find ways to abstract the logic. |
85f00ae
to
b396f0f
Compare
Let me know how :( my latest commit is. |
I think for the sake of API consistency, it's undoubtedly a step in the right direction. What do you think the likeliness of this issue popping up again is? I only ask because I could see it resurfacing elsewhere, so maybe we'd want to put this logic into its own module |
It definitely might. I can't think of any off hand like this that aren't already using streamRouter, though. If the time comes when we can re-use this logic, we can module out? I'll start on tidying up this PR. |
f80ff71
to
46ac928
Compare
46ac928
to
3896bd6
Compare
PTAL :) |
datastore: support streams with request.get
Looks good, thanks! |
* chore: run th generator, fix synth script * test: remove comment
[PR](googleapis/gapic-generator-typescript#878) within updated gapic-generator-typescript version 1.4.0 Committer: @summer-ji-eng PiperOrigin-RevId: 375759421 Source-Link: googleapis/googleapis@95fa72f Source-Link: googleapis/googleapis-gen@f40a343
[PR](googleapis/gapic-generator-typescript#878) within updated gapic-generator-typescript version 1.4.0 Committer: @summer-ji-eng PiperOrigin-RevId: 375759421 Source-Link: googleapis/googleapis@95fa72f Source-Link: googleapis/googleapis-gen@f40a343
RE: #750 (comment)
This adds streamRouter to the
get
method from Datastore, as talked about in #750.Here's a problem, though. streamRouter was intended for handling multiple results, like the results of a query where the user doesn't know how many to expect back. Always giving the user an array of the results to their callback makes sense there, but with
dataset.get
, the user knows exactly how many responses they will get back. So, that makes this awkward:On the bright side, the user can now get streaming results from this method:
I haven't thought of a great solution to the problem above yet. I'm leaning towards saying streamRouter isn't the right solution in its current form. Ideas welcome :)