Skip to content

Conversation

@cwxie-google
Copy link

@cwxie-google cwxie-google commented Jan 7, 2020

We're having 5 new apis and I'm adding php sample code for those new five apis.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 7, 2020
@cwxie-google
Copy link
Author

@bshaffer Hi Brent, could you please take a look at this PR or assign to the right one to review it?
Thanks

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.

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",
Copy link
Contributor

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);
Copy link
Contributor

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);
Copy link
Contributor

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);
Copy link
Contributor

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);
Copy link
Contributor

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';
Copy link
Contributor

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);
Copy link
Contributor

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();
Copy link
Contributor

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;
Copy link
Contributor

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']);
Copy link
Contributor

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']);

@product-auto-label product-auto-label bot added the samples Issues that are directly related to samples. label Aug 28, 2020
@meredithslota
Copy link
Contributor

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.

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. samples Issues that are directly related to samples.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants