Skip to content

Commit

Permalink
Apply suggestions from code review
Browse files Browse the repository at this point in the history
  • Loading branch information
bshaffer authored Nov 6, 2024
1 parent 392023b commit 8d03965
Show file tree
Hide file tree
Showing 3 changed files with 7 additions and 21 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/docs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,5 @@ jobs:
uses: GoogleCloudPlatform/php-tools/.github/workflows/doctum.yml@main
with:
title: "Google API Core (GAX) Reference Documentation"
default_version: ${{ inputs.tag || github.head_ref || github.event.pull_request.head.repo.full_name }}
default_version: ${{ inputs.tag || github.head_ref || github.ref_name }}
dry_run: ${{ github.event_name == 'pull_request' }}
4 changes: 1 addition & 3 deletions src/GapicClientTrait.php
Original file line number Diff line number Diff line change
Expand Up @@ -218,8 +218,6 @@ protected function getCredentialsWrapper()
*/
private function setClientOptions(array $options)
{
$hasEmulator = $options['hasEmulator'] ?? false;

This comment has been minimized.

Copy link
@lcobucci

lcobucci Nov 12, 2024

@bshaffer moving this below would be alright if $options weren't overridden by $options = new ClientOptions($options);.

Would you please revert this refactoring?

This comment has been minimized.

Copy link
@bshaffer

bshaffer Nov 12, 2024

Author Contributor

Hmm, so no test was written to prevent a single line being moved from breaking the functionality? And no comment to explain as much? We will want to add both of those things.

This comment has been minimized.

Copy link
@bshaffer

bshaffer Nov 14, 2024

Author Contributor

Done in #594


// serviceAddress is now deprecated and acts as an alias for apiEndpoint
if (isset($options['serviceAddress'])) {
$options['apiEndpoint'] = $this->pluck('serviceAddress', $options, false);
Expand Down Expand Up @@ -316,7 +314,7 @@ private function setClientOptions(array $options)
$transport,
$options['transportConfig'],
$options['clientCertSource'],
$hasEmulator
$options['hasEmulator'] ?? false
);
}

Expand Down
22 changes: 5 additions & 17 deletions src/InsecureRequestBuilder.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,30 +10,18 @@
*/
class InsecureRequestBuilder extends RequestBuilder
{
/**
* @param string $baseUri
* @param string $restConfigPath
* @throws ValidationException
*/
public function __construct(string $baseUri, string $restConfigPath)
{
parent::__construct($baseUri, $restConfigPath);
}

/**
* @param string $path
* @param array $queryParams
* @return UriInterface
*/
protected function buildUri(string $path, array $queryParams)
{
$uri = Utils::uriFor(
sprintf(
'http://%s%s',
$this->baseUri,
$path
)
);
$uri = Utils::uriFor(sprintf(
'http://%s%s',
$this->baseUri,
$path
));
if ($queryParams) {
$uri = $this->buildUriWithQuery(
$uri,
Expand Down

0 comments on commit 8d03965

Please sign in to comment.