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

First step to TypeScript upgrade #731

Merged
merged 2 commits into from
Apr 25, 2017
Merged

First step to TypeScript upgrade #731

merged 2 commits into from
Apr 25, 2017

Conversation

JustinBeckwith
Copy link
Contributor

First steps towards #503. This simply renames all the files (including generated ones) to *.ts, and makes any changes necessary to get the build and tests to pass. I am going to update #503 with the full list of steps we need to take here.

  • npm test succeeds
  • Pull request has been squashed into 1 commit
  • I did NOT manually make changes to anything in apis/
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate JSDoc comments were updated in source code (if applicable)
  • Appropriate changes to readme/docs are included in PR

I chose to hand modify apis/ to mollify the TypeScript compiler, and because I didn't want to complicate the PR with schema changes.

Sorry, something went wrong.

Verified

This commit was signed with the committer’s verified signature.
XSAM Sam Xie
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 24, 2017

Verified

This commit was signed with the committer’s verified signature.
XSAM Sam Xie
@codecov-io
Copy link

codecov-io commented Apr 24, 2017

Codecov Report

Merging #731 into master will decrease coverage by 15.13%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #731       +/-   ##
===========================================
- Coverage   94.71%   79.58%   -15.14%     
===========================================
  Files          20        8       -12     
  Lines        1382      240     -1142     
  Branches        0       53       +53     
===========================================
- Hits         1309      191     -1118     
+ Misses         73       30       -43     
- Partials        0       19       +19
Impacted Files Coverage Δ
lib/auth/computeclient.ts 100% <ø> (ø)
lib/utils.ts 100% <ø> (ø)
lib/transporters.ts 100% <ø> (ø)
lib/auth/jwtclient.ts 100% <ø> (ø)
lib/discovery.ts 67.64% <100%> (ø)
lib/apirequest.ts 97.29% <100%> (ø)
lib/googleapis.ts 78.18% <100%> (ø)
lib/generator_utils.ts 60% <100%> (ø)
... and 1 more

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 4a4cf04...6efcf1d. Read the comment docs.

@JustinBeckwith
Copy link
Contributor Author

@matthewloring since @ofrobots is out, can you take a look?

"lint": "semistandard \"**/*.js\"",
"generate-apis": "node scripts/generate",
"lint": "semistandard \"samples/**/*.js\"",
"generate-apis": "npm run build-tools && node scripts/generate.js",

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@lukesneeringer
Copy link

lukesneeringer commented Apr 24, 2017

This simply renames all the files (including generated ones) to *.ts

This seems wrong to me unless the thing that generates the code has been updated to spit out *.ts files. Hand-edits to autogen code need to not happen. (And yes, renaming the file is totally a hand-edit.)

@JustinBeckwith
Copy link
Contributor Author

@lukesneeringer I updated the generators and templates to emit *.ts files, re-ran the generator, and ensured that it all worked. The problem is that regenerating the ts files is non-deterministic. The PR would include surface changes, and about 200k lines across all generated files to review.

It's really up to you guys. I'm trying to limit the scope and improve the readability of the PR - but I'm happy to throw a watermelon over the fence :)

@lukesneeringer
Copy link

If you updated the generator, that resolves my complaint.

Copy link

@matthewloring matthewloring left a comment

Choose a reason for hiding this comment

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

It looks like a few files (auth and some others) did not go through the s/module.exports/export change. Why were these omitted?

@@ -59,13 +58,14 @@ function makeMethod (schema, method, context) {
params: params,
requiredParams: method.parameterOrder || [],
pathParams: getPathParams(method.parameters),
context: context
context: context,
mediaUrl: null

This comment was marked as spam.

This comment was marked as spam.

@@ -131,10 +133,10 @@ describe('Clients', function () {
it('should support default params', function (done) {
var google = new googleapis.GoogleApis();
var datastore = google.datastore({
version: 'v1beta2',
version: 'v1beta3',

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@jmdobry
Copy link
Contributor

jmdobry commented Apr 25, 2017

I'm going to test this branch against our Node.js samples that use google-api-nodejs-client

@jmdobry
Copy link
Contributor

jmdobry commented Apr 25, 2017

Tested against our googleapis samples in the nodejs-docs-samples repo.

@jmdobry jmdobry merged commit 5e22af7 into googleapis:master Apr 25, 2017
@JustinBeckwith JustinBeckwith deleted the ts-convert-rename branch March 11, 2018 02:47
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

6 participants