Skip to content

feat: move to Typescript code generation#631

Merged
xiaozhenliu-gg5 merged 17 commits intomasterfrom
move_to_ts
Mar 10, 2020
Merged

feat: move to Typescript code generation#631
xiaozhenliu-gg5 merged 17 commits intomasterfrom
move_to_ts

Conversation

@xiaozhenliu-gg5
Copy link
Contributor

@xiaozhenliu-gg5 xiaozhenliu-gg5 commented Mar 6, 2020

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 •
edited
Loading

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

xiaozhenliu-gg5 commented Mar 7, 2020

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

bcoe
bcoe approved these changes Mar 9, 2020
View reviewed changes
Copy link

@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.

JustinBeckwith
JustinBeckwith suggested changes Mar 9, 2020
View reviewed changes
@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

JustinBeckwith
JustinBeckwith approved these changes Mar 9, 2020
View reviewed changes
This was referenced Mar 9, 2020
Closed
Closed
summer-ji-eng
summer-ji-eng approved these changes Mar 9, 2020
View reviewed changes
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
alexander-fenster reviewed Mar 10, 2020
View reviewed changes
@alexander-fenster
Copy link
Contributor

alexander-fenster commented Mar 10, 2020 •
edited
Loading

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

alexander-fenster
alexander-fenster reviewed Mar 10, 2020
View reviewed changes
alexander-fenster
alexander-fenster approved these changes Mar 10, 2020
View reviewed changes
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

@xiaozhenliu-gg5 xiaozhenliu-gg5 merged commit 6749aa5 into master Mar 10, 2020
@xiaozhenliu-gg5 xiaozhenliu-gg5 deleted the move_to_ts branch March 10, 2020 17:30
@AVaksman AVaksman mentioned this pull request Mar 24, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@summer-ji-eng summer-ji-eng summer-ji-eng approved these changes

+3 more reviewers

@bcoe bcoe bcoe approved these changes

@JustinBeckwith JustinBeckwith JustinBeckwith approved these changes

@alexander-fenster alexander-fenster alexander-fenster approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@xiaozhenliu-gg5 @steffnay @alexander-fenster @bcoe @JustinBeckwith @summer-ji-eng @googlebot