-
Notifications
You must be signed in to change notification settings - Fork 1k
Add real time feed api sample code #1007
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
Conversation
|
@bshaffer Hi Brent, could you please take a look at this PR or assign to the right one to review it? |
bshaffer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff! mostly minor changes to the sample formats and variable name consistency.
| "src/export_assets.php", | ||
| "src/batch_get_assets_history.php" | ||
| "src/batch_get_assets_history.php", | ||
| "src/create_feed.php", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: whitespace here
| $feedOutputConfig = new FeedOutputConfig(); | ||
| $pubsubDestination = new PubsubDestination(); | ||
| $pubsubDestination->setTopic($topic); | ||
| $feedOutputConfig->setPubsubDestination($pubsubDestination); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, but let's have this ordered better, like this:
$pubsubDestination = (new PubsubDestination())
->setTopic($topic);
$feedOutputConfig = (new FeedOutputConfig())
->setPubsubDestination($pubsubDestination);
$feed = (new Feed())
->setAssetNames($assetNames)
->setFeedOutputConfig($feedOutputConfig);| { | ||
| $client = new AssetServiceClient(); | ||
|
|
||
| $client->DeleteFeed($feedName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
casing should be deleteFeed
| $feed->setAssetNames($assetNames); | ||
| $feed->setFeedOutputConfig($feedOutputConfig); | ||
|
|
||
| $createdFeed = $client->CreateFeed($parent, $feedId, $feed); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
casing should be createFeed
| { | ||
| $client = new AssetServiceClient(); | ||
|
|
||
| $feed = $client->GetFeed($feedName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
casing should be getFeed
|
|
||
| $feed = $client->ListFeeds($parent); | ||
|
|
||
| echo 'Feeds listed'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No feeds were actually listed, would probably make more sense like this:
$feeds = $client->listFeeds($parent);
printf('Feeds for %s:' . PHP_EOL, $parent);
foreach ($feeds as $feed) {
print($feed->getName() . PHP_EOL);
}| $updateMask = new FieldMask(); | ||
| $updateMask->setPaths(['asset_names']); | ||
|
|
||
| $updated_feed = $client->UpdateFeed($new_feed, $updateMask); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
casing should be updateFeed for the method call and $updatedFeed for the variable
| { | ||
| $client = new AssetServiceClient(); | ||
|
|
||
| $new_feed = new Feed(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's be consistent with our casing and make this variable $newFeed
|
|
||
| $updated_feed = $client->UpdateFeed($new_feed, $updateMask); | ||
|
|
||
| echo 'Feed Updated ' . $updated_feed; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this 'Feed Updated: ' . $updatedFeed . PHP_EOL;. All echo/print statements should have an appended new line.
| $new_feed->setName($feedName); | ||
| $new_feed->setAssetNames($assetNames); | ||
| $updateMask = new FieldMask(); | ||
| $updateMask->setPaths(['asset_names']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar issue here, let's format these like follows:
$newFeed = (new Feed())
->setName($feedName)
->setAssetNames($assetNames);
$updateMask = (new FieldMask())
->setPaths(['asset_names']);|
This PR is quite stale at this point, and the Asset API has autogenerated samples that also cover these use cases here: https://github.com/googleapis/google-cloud-php/tree/main/Asset/samples/V1/AssetServiceClient Closing this accordingly, please reopen if this is still active. |
We're having 5 new apis and I'm adding php sample code for those new five apis.