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

refactor(ts): generate logging types from proto #314

Merged
merged 2 commits into from Nov 12, 2018

Conversation

ofrobots
Copy link
Contributor

@ofrobots ofrobots commented Nov 7, 2018

Alternative to hand-writing the type-definitions #309.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 7, 2018
@ofrobots
Copy link
Contributor Author

ofrobots commented Nov 7, 2018

The second step would be to check-in the generated proto/ directory. The proto typedefs will need to be updated very rarely, so it is not necessary to generate them on each CI.

@codecov
Copy link

codecov bot commented Nov 7, 2018

Codecov Report

Merging #314 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #314   +/-   ##
=======================================
  Coverage   92.23%   92.23%           
=======================================
  Files          14       14           
  Lines         644      644           
  Branches       57       57           
=======================================
  Hits          594      594           
  Misses         36       36           
  Partials       14       14

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 4a9a7ee...c6fbace. Read the comment docs.

@ofrobots
Copy link
Contributor Author

ofrobots commented Nov 8, 2018

While we are here, we should also address https://github.com/googleapis/nodejs-logging/issues/133.

@ofrobots
Copy link
Contributor Author

ofrobots commented Nov 8, 2018

Re: #133, I believe the dynamic loadProto in

can be replaced with a load of the statically generated proto/logging.js above. /cc @alexander-fenster

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

3 participants