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

Partial veneer GAPIC refresh #600

Merged
merged 8 commits into from
Aug 21, 2017

Conversation

michaelbausor
Copy link
Contributor

cc @garrettjonesgoogle @lukesneeringer @shinfan

Toolkit PR: googleapis/gapic-generator#1428

This PR shows the expected changes from implementing partial veneers in PHP. It only updates the Vision API, and does not yet include the manual methods that will be added.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 25, 2017
@jdpedrie
Copy link
Contributor

sorry for the noise. didn't mean to click approve on this. -_-

*/
class ImageAnnotatorClient
class ImageAnnotatorClient extends ImageAnnotatorGapic

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@@ -60,7 +60,7 @@
*
* @experimental
*/
class ImageAnnotatorGapic
class ImageAnnotatorGapicClient

This comment was marked as spam.

@michaelbausor michaelbausor changed the title WIP: Partial veneer changes for vision Partial veneer GAPIC refresh Aug 1, 2017
@michaelbausor
Copy link
Contributor Author

@dwsupplee @jdpedrie I would like to move forward with this PR. I realize it is kind of a nightmare to review, because so many things change. The manual changes that definitely need review are changes in src/Spanner/Connection/Grpc.php that are necessary after the update to GAX and protobuf 3.4, and changes in tests/unit/Core/GrpcRequestWrapperTest.php, which are necessary to allow unit tests to run using the protobuf c extension, because c classes seem incompatible with prophecy.

When it comes to the rest of the PR, I am not sure what approach to take - I am happy to try to make changes to help with review.

Also note that this PR will fail tests until GAX 0.22.1 is released (pending this PR: googleapis/gax-php#94)

@michaelbausor
Copy link
Contributor Author

Also, if possible it would be great to get another pair of eyes on googleapis/gax-php#94 if you have a chance

Copy link
Contributor

@dwsupplee dwsupplee left a comment

Choose a reason for hiding this comment

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

I didn't spend much time on the generated clients as it looks like the only change was the inherit tag and the extension. Please let me know if otherwise.

Everything else looks good to me, with one thing I am curious about - If I were a new user coming in to this ecosystem why do I choose DlpServiceClient over DlpServiceGapicClient?

To help answer that, what do you think about moving the *-GapicClients to their own directory - or is there a clear use case for having them both be exposed at the same level?

@michaelbausor
Copy link
Contributor Author

I don't think there is a clear use case - I would expect users to never interact with the GapicClient objects. I am happy to move the gapic client into its own folder. My first thought is:

- Dlp
  - V2beta1
    - DlpServiceClient.php
    - Gapic
      - DlpServiceGapicClient.php

but I could also see this working:

- Dlp
  - V2beta1
    - DlpServiceClient.php
  - Gapic
    - V2beta1
      - DlpServiceGapicClient.php

Which do you think is better?

@dwsupplee
Copy link
Contributor

My vote goes for option 1 :)

@dwsupplee dwsupplee merged commit 7b05f81 into googleapis:master Aug 21, 2017
@jdpedrie jdpedrie mentioned this pull request Aug 25, 2017
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.

5 participants