Skip to content
This repository was archived by the owner on Jul 20, 2023. It is now read-only.

feat: adds the v1 API surface#134

Merged
bcoe merged 3 commits into
masterfrom
v1-surface
Mar 24, 2020
Merged

feat: adds the v1 API surface#134
bcoe merged 3 commits into
masterfrom
v1-surface

Conversation

@bcoe

@bcoe bcoe commented Mar 24, 2020

Copy link
Copy Markdown

runs the generator with the v1 API surface.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 24, 2020
@codecov

codecov Bot commented Mar 24, 2020

Copy link
Copy Markdown

Codecov Report

Merging #134 into master will increase coverage by 1.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #134      +/-   ##
==========================================
+ Coverage   57.61%   58.62%   +1.01%     
==========================================
  Files           2        2              
  Lines       13786    18841    +5055     
  Branches        1        1              
==========================================
+ Hits         7943    11046    +3103     
- Misses       5843     7795    +1952
Impacted Files Coverage Δ
protos/protos.js 58.56% <ø> (+1.02%) ⬆️
src/index.ts 100% <100%> (ø) ⬆️

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 f784696...9e05cec. Read the comment docs.

@bcoe bcoe added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 24, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 24, 2020

@JustinBeckwith JustinBeckwith left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM w/ nits. Do we know if v1 / v1beta1 has breaking changes in it?

Comment thread .github/workflows/ci.yaml
node: [8, 10, 12, 13]
steps:
- uses: actions/checkout@v1
- uses: actions/checkout@v2

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could I trouble you for a hammer PR on this one? We shouldn't be rename/merging this 80 times :)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

this PR has already gone out to all repositories, and is the one Alex is working on landing as we speak.

Comment thread src/index.ts Outdated
// For compatibility with JavaScript libraries we need to provide this default export:
// tslint:disable-next-line no-default-export
export default {v1beta1, WebRiskServiceV1Beta1Client};
export default {v1, v1beta1, WebRiskServiceV1Beta1Client};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't understand what this line of code does. Do you have a good grip on why/if this is needed?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This approach is necessary to ensure that the module has the same export signature as the JavaScript library did prior to the TypeScript conversion:

{
  TextToSpeechClient: [Function: TextToSpeechClient],
  v1beta1: { TextToSpeechClient: [Function: TextToSpeechClient] },
  v1: [Circular],
  default: {
    TextToSpeechClient: [Function: TextToSpeechClient],
    v1beta1: { TextToSpeechClient: [Function: TextToSpeechClient] },
    v1: [Circular]
  }
}

@bcoe

bcoe commented Mar 24, 2020

Copy link
Copy Markdown
Author

the v1 and v1beta surfaces are identical, I learned talking with @bsurmanski. So I've updated samples accordingly.

@bcoe bcoe merged commit 34ca53c into master Mar 24, 2020
@bcoe bcoe deleted the v1-surface branch March 24, 2020 17:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants