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.

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

fix: correct method for supportedTransports #692

Merged
merged 3 commits into from
Feb 28, 2024
Merged

Conversation

bshaffer
Copy link
Contributor

@bshaffer bshaffer commented Feb 28, 2024

Woops! We've been writing the wrong method name all this time. The method is actually supportedTransports, but we've been generating getSupportedTransports. OH NO!

See ClientOptionsTrait::supportedTransports (called here)

@bshaffer bshaffer requested review from a team as code owners February 28, 2024 15:12
Copy link
Contributor

@ajupazhamayil ajupazhamayil left a comment

Choose a reason for hiding this comment

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

Great catch. LGTM.

@@ -229,8 +229,8 @@ private static function defaultTransport()
return 'rest';
}

/** Implements GapicClientTrait::getSupportedTransports. */
private static function getSupportedTransports()
/** Implements GapicClientTrait::supportedTransports. */
Copy link
Contributor

@ajupazhamayil ajupazhamayil Feb 28, 2024

Choose a reason for hiding this comment

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

Curious Q: How did you catch this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was reviewing @yash30201 's doc on Storage V2, and how it only will support grpc, so I wanted to check what the method was that we'd need to generate (which is this one), and noticed the names were different between here and GAX.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

...it would be better if we had a test, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test added!

d32d4ba

@bshaffer bshaffer merged commit e7e08da into main Feb 28, 2024
5 checks passed
@bshaffer bshaffer deleted the fix-supported-transports branch February 28, 2024 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants