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.

Sign up for GitHub

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

Jump to bottom

feat: introduce snippet gen support #483

Merged
merged 41 commits into from
Oct 6, 2022
Merged

feat: introduce snippet gen support #483

merged 41 commits into from
Oct 6, 2022

Conversation

dwsupplee
Copy link
Contributor

@dwsupplee dwsupplee commented May 27, 2022 •
edited by bshaffer
Loading

No description provided.

@google-cla
Copy link

google-cla bot commented May 27, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@snippet-bot
Copy link

snippet-bot bot commented May 27, 2022 •
edited
Loading

Here is the summary of changes.

You are about to add 317 region tags.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

@dwsupplee dwsupplee force-pushed the snippet-gen branch 2 times, most recently from 690ed51 to 2e84f2b Compare May 27, 2022 20:45
@dwsupplee dwsupplee mentioned this pull request Jun 23, 2022
Closed
bshaffer
bshaffer reviewed Aug 12, 2022
View reviewed changes
Copy link
Contributor

@bshaffer bshaffer left a comment

Choose a reason for hiding this comment

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

Preliminary pass with some minor suggestions

src/Ast/PhpFile.php Outdated Show resolved Hide resolved
src/Ast/PhpFile.php Show resolved Hide resolved
src/Generation/SnippetGenerator.php Outdated Show resolved Hide resolved
tests/Unit/ProtoTests/BasicBidiStreaming/out/samples/BasicBidiStreamingClient/method_bidi.php Outdated Show resolved Hide resolved
tests/Unit/ProtoTests/BasicBidiStreaming/out/samples/BasicBidiStreamingClient/method_bidi.php Show resolved Hide resolved
tests/Unit/ProtoTests/BasicBidiStreaming/out/samples/BasicBidiStreamingClient/method_empty.php Outdated Show resolved Hide resolved
...Unit/ProtoTests/BasicServerStreaming/out/samples/BasicServerStreamingClient/method_empty.php Outdated Show resolved Hide resolved
tests/Unit/ProtoTests/Kms/out/samples/V1/KeyManagementServiceClient/get_crypto_key.php Outdated
use Google\Cloud\Kms\V1\KeyManagementServiceClient;

/**
* Returns metadata for a given [CryptoKey][google.cloud.kms.v1.CryptoKey], as
Copy link
Contributor

Choose a reason for hiding this comment

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

This format is common in protos, but for PHP specifically we may be better off using @see tags, as I believe it's more idiomatic. Also the proto name (in this case google.cloud.kms.v1.CryptoKey) is not really relevant.

 * Returns metadata for a given {@see CryptoKey}, as

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 - There is a todo here for something like this when possible, https://github.com/googleapis/gapic-generator-php/pull/483/files#diff-6d2583711a56ed5f6ed9262793b7185f0bc017a49a35580b8056429bb2d491fcR161

.../Unit/ProtoTests/Kms/out/samples/V1/KeyManagementServiceClient/import_crypto_key_version.php Outdated Show resolved Hide resolved
noahdietz
noahdietz reviewed Aug 12, 2022
View reviewed changes
src/Generation/SnippetDetails.php Outdated Show resolved Hide resolved
src/Generation/SnippetGenerator.php Outdated Show resolved Hide resolved
src/Generation/SnippetGenerator.php Outdated Show resolved Hide resolved
src/CodeGenerator.php Outdated Show resolved Hide resolved
@dwsupplee dwsupplee added the cla: yes This human has signed the Contributor License Agreement. label Sep 26, 2022
@dwsupplee dwsupplee marked this pull request as ready for review September 26, 2022 04:46
@dwsupplee dwsupplee requested review from a team as code owners September 26, 2022 04:46
@dwsupplee
Copy link
Contributor Author

/cc @amanda-tarafa PTAL :).

bshaffer
bshaffer reviewed Sep 29, 2022
View reviewed changes
src/Generation/FieldDetails.php Outdated Show resolved Hide resolved
src/Generation/FieldDetails.php Show resolved Hide resolved
bshaffer
bshaffer reviewed Sep 30, 2022
View reviewed changes
...ion/goldens/dataproc/samples/V1/AutoscalingPolicyServiceClient/create_autoscaling_policy.php Outdated
* has the following format: `projects/{project_id}/regions/{region}` * For
* `projects.locations.autoscalingPolicies.create`, the resource name of the
* location has the following format: `projects/{project_id}/locations/{location}`
* Please see {@see AutoscalingPolicyServiceClient::regionName()} for help
Copy link
Contributor

Choose a reason for hiding this comment

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

😍

amanda-tarafa
amanda-tarafa approved these changes Oct 4, 2022
View reviewed changes
Copy link

@amanda-tarafa amanda-tarafa left a comment

Choose a reason for hiding this comment

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

LGTM from the point of view of SnippetGen Phase 1 output.

Thanks!!

tests/Integration/goldens/asset/samples/V1/AssetServiceClient/analyze_iam_policy.php Show resolved Hide resolved
tests/Integration/goldens/asset/samples/V1/AssetServiceClient/batch_get_assets_history.php Show resolved Hide resolved
amanda-tarafa
amanda-tarafa requested changes Oct 4, 2022
View reviewed changes
Copy link

@amanda-tarafa amanda-tarafa left a comment

Choose a reason for hiding this comment

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

Undoing LGTM, as I just thought to check for two issues that were recently surfaced on all other generators. It's worth it to ship PHP with them fixed.

Thanks!

tests/Integration/goldens/iam/samples/V1/IAMPolicyClient/get_iam_policy.php Outdated Show resolved Hide resolved
tests/Integration/goldens/iam/samples/V1/IAMPolicyClient/get_iam_policy.php Show resolved Hide resolved
noahdietz
noahdietz approved these changes Oct 4, 2022
View reviewed changes
Copy link
Collaborator

@noahdietz noahdietz left a comment

Choose a reason for hiding this comment

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

General LGTM

amanda-tarafa
amanda-tarafa approved these changes Oct 6, 2022
View reviewed changes
Copy link

@amanda-tarafa amanda-tarafa left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@dwsupplee dwsupplee merged commit 0a52d7e into googleapis:main Oct 6, 2022
@release-please release-please bot mentioned this pull request Oct 6, 2022
Merged
@dwsupplee dwsupplee mentioned this pull request Nov 15, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bshaffer bshaffer bshaffer approved these changes

@amanda-tarafa amanda-tarafa amanda-tarafa approved these changes

@noahdietz noahdietz noahdietz approved these changes

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.
5 participants
@dwsupplee @bshaffer @noahdietz @amanda-tarafa @renovate-bot