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

feat: convert to TypeScript - #236

Merged
alexander-fenster merged 9 commits into
masterfrom
move_typescript
Nov 22, 2019
Merged

feat: convert to TypeScript#236
alexander-fenster merged 9 commits into
masterfrom
move_typescript

Conversation

@xiaozhenliu-gg5

@xiaozhenliu-gg5 xiaozhenliu-gg5 commented Nov 21, 2019 •
edited by alexander-fenster
Loading

Copy link
Copy Markdown
Contributor

Converting redis to TypeScript. This should be totally non-breaking.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 21, 2019
@alexander-fenster

alexander-fenster commented Nov 21, 2019

Copy link
Copy Markdown
Contributor

Please do not merge until we figure out what to do with listInstances method which stopped being auto-paginated with the new annotations. It's the only breaking change here.

@alexander-fenster alexander-fenster added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Nov 21, 2019
@alexander-fenster alexander-fenster changed the title feat!: move library to typescript code generation DO NOT MERGE: feat!: move library to typescript code generation Nov 21, 2019
@alexander-fenster alexander-fenster mentioned this pull request Nov 21, 2019
Merged
@codecov

codecov Bot commented Nov 22, 2019 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #236   +/-   ##
=========================================
  Coverage          ?   92.51%           
=========================================
  Files             ?        5           
  Lines             ?     2645           
  Branches          ?       89           
=========================================
  Hits              ?     2447           
  Misses            ?      196           
  Partials          ?        2
Impacted Files Coverage Δ
src/index.ts 100% <100%> (ø)
src/v1beta1/index.ts 100% <100%> (ø)
src/v1/index.ts 100% <100%> (ø)
src/v1/cloud_redis_client.ts 92.32% <92.32%> (ø)
src/v1beta1/cloud_redis_client.ts 92.33% <92.33%> (ø)

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 d6c7f4d...2e1cc68. Read the comment docs.

@alexander-fenster alexander-fenster changed the title DO NOT MERGE: feat!: move library to typescript code generation feat: move library to typescript code generation Nov 22, 2019
@alexander-fenster alexander-fenster changed the title feat: move library to typescript code generation feat: convert to TypeScript Nov 22, 2019
bcoe
bcoe approved these changes Nov 22, 2019
View reviewed changes

@bcoe bcoe left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this is looking good to me, once we address the breaking change 👍

Comment thread package.json Outdated
"lint": "eslint '**/*.js'",
"test": "c8 mocha build/test",
"samples-test": "cd samples/ && npm link ../ && npm install && npm test && cd ../",
"system-test": "mocha build/system-test",

@bcoe bcoe Nov 22, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

to give us a full picture of coverage, I think we should gravitate towards using c8 for running system-test and sample-tests, codecov merges reports.

@alexander-fenster alexander-fenster Nov 22, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

OK! updated package.json

Comment thread synth.py
s.copy(
library,
excludes=['package.json', 'README.md', 'src/index.js'])
excludes=['package.json', 'README.md', 'src/index.ts'])

@bcoe bcoe Nov 22, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

were there manual changes in src/index.ts? if not we should avoid excluding it.

@alexander-fenster alexander-fenster Nov 22, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, we are merging two versions there. We don't do it automatically yet.

@alexander-fenster

alexander-fenster commented Nov 22, 2019

Copy link
Copy Markdown
Contributor

What kind of breaking change?

@bcoe

bcoe commented Nov 22, 2019

Copy link
Copy Markdown

@alexander-fenster you mentioned listInstances was currently a breaking change, was the goal to make it not one, or will this be a major?

@alexander-fenster

alexander-fenster commented Nov 22, 2019

Copy link
Copy Markdown
Contributor

@bcoe Ah! That one is fixed! googleapis/gapic-generator-typescript#156 fixed that, there are no other breaking changes I'm aware of here. :)

@bcoe

bcoe commented Nov 22, 2019

Copy link
Copy Markdown

@alexander-fenster 👍 awesome, land at will; any reason to keep the blocked tag on?

@alexander-fenster alexander-fenster removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Nov 22, 2019
@alexander-fenster

alexander-fenster commented Nov 22, 2019

Copy link
Copy Markdown
Contributor

Nope! Landing now.

@alexander-fenster
alexander-fenster merged commit 6bcdf55 into master Nov 22, 2019
@alexander-fenster
alexander-fenster deleted the move_typescript branch November 22, 2019 19:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@JustinBeckwith JustinBeckwith Awaiting requested review from JustinBeckwith
2 more reviewers
@alexander-fenster alexander-fenster alexander-fenster left review comments
@bcoe bcoe bcoe 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.

4 participants

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