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

feat: move API to typescript code generation#135

Merged
xiaozhenliu-gg5 merged 37 commits intomasterfrom
move_to_ts
Feb 7, 2020
Merged

feat: move API to typescript code generation#135
xiaozhenliu-gg5 merged 37 commits intomasterfrom
move_to_ts

Conversation

@xiaozhenliu-gg5
Copy link
Contributor

@xiaozhenliu-gg5 xiaozhenliu-gg5 commented Feb 4, 2020

No description provided.

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

codecov bot commented Feb 4, 2020 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@d28f611). Click here to learn what that means.
The diff coverage is 60.47%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #135   +/-   ##
=========================================
  Coverage          ?   69.27%           
=========================================
  Files             ?        6           
  Lines             ?     3437           
  Branches          ?       73           
=========================================
  Hits              ?     2381           
  Misses            ?     1054           
  Partials          ?        2
Impacted Files Coverage Δ
src/v1beta1/grafeas_v1_beta1_client.ts 50.98% <36.92%> (ø)
src/v1/container_analysis_client.ts 87.41% <73.17%> (ø)
src/v1beta1/container_analysis_v1_beta1_client.ts 88.93% <77.04%> (ø)

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 d28f611...12585e6. Read the comment docs.

@xiaozhenliu-gg5
Copy link
Contributor Author

xiaozhenliu-gg5 commented Feb 4, 2020

Can I get some advice of synth.py now it has too many post-processing for removing files or replacing file content, is there a faster way to do that? @alexander-fenster Thanks!

alexander-fenster
alexander-fenster reviewed Feb 4, 2020
View reviewed changes
alexander-fenster
alexander-fenster reviewed Feb 4, 2020
View reviewed changes
alexander-fenster
alexander-fenster reviewed Feb 4, 2020
View reviewed changes
alexander-fenster
alexander-fenster reviewed Feb 4, 2020
View reviewed changes
alexander-fenster
alexander-fenster reviewed Feb 4, 2020
View reviewed changes
alexander-fenster
alexander-fenster reviewed Feb 4, 2020
View reviewed changes
@alexander-fenster
Copy link
Contributor

alexander-fenster commented Feb 4, 2020

OK so removal of grafeas files can be done with a loop as I wrote in comments above. As for the "surgery", I'm thinking of maybe trying a mixin here? Let's talk tomorrow and see if we can make it in a more gentle way.

JustinBeckwith
JustinBeckwith approved these changes Feb 5, 2020
View reviewed changes
Copy link
Contributor

@JustinBeckwith JustinBeckwith left a comment

Choose a reason for hiding this comment

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

synth is funky, but generally LGTM

alexander-fenster
alexander-fenster reviewed Feb 7, 2020
View reviewed changes
src/v1/container_analysis_client.ts
*
* The second parameter to the callback is an object representing [Policy]{@link google.iam.v1.Policy}.
* @param {object} [options]
* Call options. See {@link https://googleapis.dev/nodejs/google-gax/latest/interfaces/CallOptions.html|CallOptions} for more details.
Copy link
Contributor

@alexander-fenster alexander-fenster Feb 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Interesting: I wonder why we lost those extra comments for each field in the request object. That's OK for now, but may I ask you @xiaozhenliu-gg5 to create an issue to figure out if we can have those comments back if it's not too hard? They look useful.

Copy link
Contributor Author

@xiaozhenliu-gg5 xiaozhenliu-gg5 Feb 7, 2020

Choose a reason for hiding this comment

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

sure, created an issue, will take a look

alexander-fenster
alexander-fenster reviewed Feb 7, 2020
View reviewed changes
alexander-fenster
alexander-fenster reviewed Feb 7, 2020
View reviewed changes
alexander-fenster
alexander-fenster approved these changes Feb 7, 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 small nits

@xiaozhenliu-gg5 xiaozhenliu-gg5 mentioned this pull request Nov 4, 2025
Closed
@xiaozhenliu-gg5 xiaozhenliu-gg5 merged commit b2d88af into master Feb 7, 2020
@xiaozhenliu-gg5 xiaozhenliu-gg5 deleted the move_to_ts branch February 7, 2020 20:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

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.

5 participants

@xiaozhenliu-gg5 @alexander-fenster @bcoe @JustinBeckwith @googlebot