Conversation
…erived correctly - Support Google\Ads namespace by using vendor googleads/, hyphenating project name, and setting github repo to googleapis/php-ads-* - Support CommonProtos by detecting missing protobuf service definitions, stripping type/common subpackages, appending CommonProtos to component name, and setting repo metadata library_type to CORE - Allow empty api_shortname when no default_host is defined Fixes #9387
bshaffer
left a comment
bshaffer
left a comment
There was a problem hiding this comment.
This looks good but the changes are way too complex for what I had in mind. I think there are a lot of opportunities to simplify things
| if (str_starts_with($phpNamespace, 'Google\\Ads')) { | ||
| if (count($parts) > 1 && 'v' === strtolower($parts[count($parts) - 1][0] ?? '')) { | ||
| array_pop($parts); | ||
| } | ||
| $name = str_replace( | ||
| ['google.', 'devtools.cloud', '.'], | ||
| ['', 'cloud-', '-'], | ||
| implode('.', $parts) | ||
| ); | ||
| if (str_starts_with($name, 'ads-')) { | ||
| $name = substr($name, 4); | ||
| } | ||
| if (str_ends_with($name, 'manager') && !str_ends_with($name, '-manager')) { | ||
| $name = substr($name, 0, -7) . '-manager'; | ||
| } | ||
| if (!$hasGapicClient && !str_ends_with($name, '-common-protos')) { | ||
| $name .= '-common-protos'; | ||
| } | ||
| return 'googleads/' . $name; | ||
| } |
There was a problem hiding this comment.
This is overly complicated. We don't need special handling for all of these cases, but rather should just use googleads instead of google if the phpNamespace starts with Google\Ads
| while (count($parts) > 1) { | ||
| $last = end($parts); | ||
| if ('v' === $last[0]) { | ||
| array_pop($parts); | ||
| } elseif (!$hasGapicClient && count($parts) > 2 && in_array($last, ['type', 'common'])) { | ||
| array_pop($parts); | ||
| } else { | ||
| break; | ||
| } |
There was a problem hiding this comment.
This is also over complicated. Could simplify to
| while (count($parts) > 1) { | |
| $last = end($parts); | |
| if ('v' === $last[0]) { | |
| array_pop($parts); | |
| } elseif (!$hasGapicClient && count($parts) > 2 && in_array($last, ['type', 'common'])) { | |
| array_pop($parts); | |
| } else { | |
| break; | |
| } | |
| $last = end($parts); | |
| if ('v' === $last[0] || in_array($last, ['type', 'common'])) { | |
| array_pop($parts); | |
| } |
| while (count($parts) > 1) { | ||
| $last = end($parts); | ||
| if ('v' === strtolower($last[0])) { | ||
| array_pop($parts); | ||
| } elseif (!$hasGapicClient && count($parts) > 2 && in_array(strtolower($last), ['type', 'common'])) { | ||
| array_pop($parts); | ||
| } else { | ||
| break; | ||
| } |
There was a problem hiding this comment.
We do not want to modify the proto namespace, we should take what the PHP namespace is from the file
| $hasGapicClient = !str_ends_with($new->componentName, 'CommonProtos'); | ||
| if (!$hasGapicClient && !str_ends_with($new->displayName, 'Common Protos')) { | ||
| $new->displayName .= ' Common Protos'; | ||
| } | ||
| $new->composerPackage = self::getComposerPackage( | ||
| $new->protoPackage, | ||
| $new->phpNamespace, | ||
| $hasGapicClient | ||
| ); |
There was a problem hiding this comment.
This logic is a bit odd, as it relies on the user to pass in CommonProtos as the component name. But if this is done, then we can also expect the user to pass in Common Protos as the display name, so there's no reason for us to write logic to do so.
There was a problem hiding this comment.
Done, removed the display name mutation from fromOptions.
| private static function getComposerPackage( | ||
| string $protoPackage, | ||
| string $phpNamespace, | ||
| bool $hasGapicClient = true |
There was a problem hiding this comment.
instead of $hasGapicClient, let's just make it $isCommonProtos
There was a problem hiding this comment.
Updated to $isCommonProtos across the class.
| return 'googleads/' . $name; | ||
| } | ||
|
|
||
| if (!$hasGapicClient && count($parts) > 2 && in_array(end($parts), ['type', 'common'])) { |
There was a problem hiding this comment.
simplify
| if (!$hasGapicClient && count($parts) > 2 && in_array(end($parts), ['type', 'common'])) { | |
| if (!$hasGapicClient && in_array(end($parts), ['type', 'common'])) { |
There was a problem hiding this comment.
Simplified as suggested.
| if (!$hasGapicClient) { | ||
| if (!str_ends_with($new->componentName, 'CommonProtos')) { | ||
| $new->componentName .= 'CommonProtos'; | ||
| } | ||
| if (!str_ends_with($new->displayName, 'Common Protos')) { | ||
| $new->displayName .= ' Common Protos'; | ||
| } |
There was a problem hiding this comment.
This logic should be in getDisplayName and getComponentName respectively
There was a problem hiding this comment.
Moved the CommonProtos naming logic into getDisplayName and getComponentName.
|
Updated to simplify per review feedback:
|
2 participants
Ensures that component parameters for Google Ads APIs and Common Protos libraries are derived correctly when running
component:new.Fixes #9387
Summary of Changes
Google\Ads):googleads/(e.g.googleads/ad-manager,googleads/data-manager,googleads/marketingplatform-admin).-managerand stripsads-prefix.googleapis/php-ads-[project-name].hasGapicClientdetection to check for protobufservicedefinitions in proto contents.typeandcommonproto and PHP namespace segments.CommonProtosto component name (e.g.GeoCommonProtos,ShoppingCommonProtos).-common-protosto Composer package and GitHub repository names.api_shortnamewithout erroring when nodefault_hostis defined in the proto.library_typetoCOREin.repo-metadata-full.json.NewComponentTestandComponentNewCommandTestcovering Ads APIs and Common Protos libraries.