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.

Sign up for GitHub

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

Jump to bottom

fix!: return API objects in streams #967

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

fix!: return API objects in streams #967

wants to merge 8 commits into from

Conversation

AVaksman
Copy link
Contributor

  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Convert objects returned by streams to API objects.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 21, 2020
@codecov
Copy link

codecov bot commented May 21, 2020 •
edited
Loading

Codecov Report

Merging #967 (e025320) into master (d851858) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master     #967    +/-   ##
========================================
  Coverage   98.60%   98.60%            
========================================
  Files          23       23            
  Lines       21923    21974    +51     
  Branches     1226     1102   -124     
========================================
+ Hits        21617    21668    +51     
  Misses        297      297            
  Partials        9        9
Impacted Files Coverage Δ
src/database.ts 99.96% <100.00%> (+<0.01%) ⬆️
src/index.ts 100.00% <100.00%> (ø)
src/instance.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d851858...e025320. Read the comment docs.

@skuruppu skuruppu requested a review from olavloite May 21, 2020 10:15
@skuruppu
Copy link
Contributor

@olavloite since you've been looking into streams recently, would you be able to help review this?

@olavloite
Copy link
Contributor

@AVaksman Do I understand this correctly that the main feature change here is that these streams will now emit the data as Spanner (rich) client classes (e.g. class Session) instead of the generated gRPC objects (e.g. interface ISession)? Won't that break user code that currently relies on these streams returning the gRPC objects?

stephenplusplus
stephenplusplus reviewed May 21, 2020
View reviewed changes
src/database.ts Outdated
) {
const session = self.session(chunk.name!);
session.metadata = chunk;
this.push(session);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe callback(null, session) would be our usual pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems neater.
Thanks!

@AVaksman
Copy link
Contributor Author

Do I understand this correctly that the main feature change here is that these streams will now emit the data as Spanner (rich) client classes (e.g. class Session) instead of the generated gRPC objects (e.g. interface ISession)?

Yes, as streaming methods should emit same data as their callback cousins.

Won't that break user code that currently relies on these streams returning the gRPC objects?

I believe this was missed in the previous PR #925 and this PR fixes it.

The documentation also sights the emitted data as Spanner (rich) client classes

   * @returns {ReadableStream} A readable stream that emits {@link Session}
   *     instances.
---------------
   * @returns {ReadableStream} A readable stream that emits {@link Instance}
   *     instances.
---------------
   * @returns {ReadableStream} A readable stream that emits {@link Backup}
   *     instances.
---------------
   * @returns {ReadableStream} A readable stream that emits {@link Database}
   *     instances.

@stephenplusplus
Copy link
Contributor

I believe this was missed in the previous PR #925 and this PR fixes it.

I would still call this breaking :( User code that worked will not work after this release.

@AVaksman AVaksman changed the title fix: return API objects in streams fix!: return API objects in streams May 21, 2020
@AVaksman
Copy link
Contributor Author

I would still call this breaking :( User code that worked will not work after this release.

Adjusted the PR header
Thanks!

@AVaksman AVaksman added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 28, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 28, 2020
@AVaksman AVaksman added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 28, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 28, 2020
@AVaksman AVaksman added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label May 28, 2020
@AVaksman AVaksman closed this Jun 19, 2020
@AVaksman AVaksman deleted the improve_streams branch June 19, 2020 13:50
@AVaksman AVaksman restored the improve_streams branch June 19, 2020 16:28
@AVaksman AVaksman reopened this Jun 19, 2020
@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/nodejs-spanner API. label Aug 21, 2020
@AVaksman AVaksman requested a review from a team as a code owner September 10, 2020 18:51
@AVaksman AVaksman requested a review from a team as a code owner September 10, 2020 18:51
@JustinBeckwith
Copy link
Contributor

@AVaksman @stephenplusplus @skuruppu this PR is looking a little moldy :) What's the plan here?

@AVaksman AVaksman added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 3, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 3, 2020
@AVaksman
Copy link
Contributor Author

AVaksman commented Nov 3, 2020

Just waiting for the next major release.

@skuruppu
Copy link
Contributor

skuruppu commented Nov 3, 2020

The next major release will come in around a month's time. We're just gathering up all the breaking changes :)

@AVaksman AVaksman added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 11, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 11, 2020
@bcoe bcoe added the owlbot:run Add this label to trigger the Owlbot post processor. label Jul 2, 2021
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jul 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stephenplusplus stephenplusplus stephenplusplus left review comments

@olavloite olavloite olavloite approved these changes

Assignees
No one assigned
Labels
api: spanner Issues related to the googleapis/nodejs-spanner API. cla: yes This human has signed the Contributor License Agreement. do not merge Indicates a pull request not ready for merge, due to either quality or timing.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@AVaksman @skuruppu @olavloite @stephenplusplus @JustinBeckwith @bcoe @googlebot @yoshi-kokoro