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

feat(deps): template change for new gts dependency #354

Closed
wants to merge 7 commits into from

Conversation

xiaozhenliu-gg5
Copy link
Contributor

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

  1. remove PaginationCallback, PaginationResponse import
  2. disable any type check for (this._protos as any)
  3. disable no-var-requires for const serviceModule = require('../src');
  4. disable no-empty-function check for class Operation in gapic-test
  5. remove unneeded const expectedResponse = {}; in gapic-test

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 24, 2020
@@ -19,6 +19,7 @@
import * as protosTypes from '../protos/protos';
import * as assert from 'assert';
import { describe, it } from 'mocha';
/* eslint-disable @typescript-eslint/no-var-requires */
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated, but I would love to have a proper import here (I remember there were some types problems when we tried it).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, Justin also brought this up in the other PR, import module will bring types in. Basically it's about mock methods like client._innerApiCalls.echo, while _innerApiCalls is private in EchoClient do we want to fix them now? Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you spend an hour (but not more) on that? If we can make innerApiCalls public (I don't see any real harm about doing it) it might become possible to import and mock it properly. If you figure out you need more changes, then let's leave it as is.

A better alternative is to move unit tests to sinon, removing all the custom mocks :) But this will take more time and we're probably not going to do it now.

@@ -185,6 +185,7 @@ export class BigQueryStorageClient {
this.bigQueryStorageStub = this._gaxGrpc.createStub(
this._opts.fallback ?
(this._protos as protobuf.Root).lookupService('google.cloud.bigquery.storage.v1beta1.BigQueryStorage') :
/* eslint-disable @typescript-eslint/no-explicit-any */
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need both eslint-disable and tsline:disable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right! :) we just need eslint-disable, thank you! also removed tslint in base PR, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it does not work without the latest version of gts. This PR was intended to make some pure template change before updating gts, but seems this is more and more overlapped with the gts PR, I may close this later on and resolve all related problems in the base PR: #343

@@ -17,7 +17,7 @@
// ** All changes to this file may be overwritten. **

import * as gax from 'google-gax';
import {APICallback, Callback, CallOptions, Descriptors, ClientOptions, LROperation, PaginationCallback, PaginationResponse} from 'google-gax';
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmmm. Maybe it's better to start actually using these types if possible? :) (in the client template, line 551 and nearby)

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.

3 participants