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

feat: move to Typescript code generation #631

Merged
merged 17 commits into from Mar 10, 2020
Merged

feat: move to Typescript code generation #631

merged 17 commits into from Mar 10, 2020

Conversation

xiaozhenliu-gg5
Copy link
Contributor

No description provided.

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

codecov bot commented Mar 6, 2020

Codecov Report

Merging #631 into master will increase coverage by 23.08%.
The diff coverage is 91.15%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #631       +/-   ##
===========================================
+ Coverage   73.13%   96.22%   +23.08%     
===========================================
  Files          33       15       -18     
  Lines       15964    12499     -3465     
  Branches      586      786      +200     
===========================================
+ Hits        11676    12027      +351     
+ Misses       4288      469     -3819     
- Partials        0        3        +3
Impacted Files Coverage Δ
src/v2/index.ts 100% <100%> (ø)
src/v2/bigtable_client.ts 91.32% <90.82%> (ø)
src/v2/bigtable_instance_admin_client.ts 91.1% <91.1%> (ø)
src/v2/bigtable_table_admin_client.ts 91.2% <91.2%> (ø)

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 9d09d37...864d786. Read the comment docs.

@xiaozhenliu-gg5
Copy link
Contributor Author

Send out cl/299452868 for fixing the timeout setting in admin/v2 which blocks system-test. Other than that, all tests are green.

Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

This looks good to me, but we should loop in @steffnay as well.

Given the popularity of this library, I would love to see if someone like Steffany who's familiar with the library could help smoke test a bit, or recommend some smoke testing we could perform.

synth.py Show resolved Hide resolved
@steffnay
Copy link

steffnay commented Mar 9, 2020

@xiaozhenliu-gg5 @bcoe This looks good to me. I'm actually not super familiar with this library, though, so I don't think I can be much help with the tests

synth.py Show resolved Hide resolved
Copy link
Contributor

@summer-ji-eng summer-ji-eng left a comment

Choose a reason for hiding this comment

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

LGTM

@alexander-fenster
Copy link
Contributor

alexander-fenster commented Mar 10, 2020

@xiaozhenliu-gg5 Please remove src/v2/.DS_Store from the change :)

synth.py Outdated Show resolved Hide resolved
Copy link
Contributor

@alexander-fenster alexander-fenster left a comment

Choose a reason for hiding this comment

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

LGTM with nits

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants