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

Extensible client options #1263

Merged
merged 7 commits into from
Jun 18, 2019
Merged

Conversation

chingor13
Copy link
Collaborator

Introduces the builder pattern for these common configuration options so that it is extensible in the future.
Add ability to configure requestReason and userAgent.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 22, 2019
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Jun 4, 2019
chingor13 added 4 commits June 5, 2019 16:41
…lient.

Introduces the builder pattern for these common configuration options so that it is extensible in the future.
@chingor13 chingor13 force-pushed the system-parameters branch from 62e32a4 to 1678fef Compare June 5, 2019 23:41
@sduskis sduskis added needs work This is a pull request that needs a little love. and removed 🚨 This issue needs some love. labels Jun 6, 2019
@chingor13 chingor13 marked this pull request as ready for review June 17, 2019 17:35
@chingor13 chingor13 requested a review from a team as a code owner June 17, 2019 17:35
@chingor13 chingor13 removed the needs work This is a pull request that needs a little love. label Jun 17, 2019
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Jun 17, 2019
@sduskis
Copy link

sduskis commented Jun 17, 2019

Should this class use Auto-Value?

@chingor13
Copy link
Collaborator Author

Should this class use Auto-Value?

We certainly could. We're not using autovalue yet for any of the other builders (or at all). It might make sense to come back with a refactoring PR to convert the all builders to use autovalue (if it's even possible). Otherwise, we will have a mix of several manually defined builders and one autovalue builder. WDYT?

@chingor13
Copy link
Collaborator Author

chingor13 commented Jun 17, 2019

@sduskis Stubbed out an autovalue implementation of CommonGoogleOptions in #1326. I'm not sure we can shim autovalue into the existing classes as they are already part of a class where users instantiate the classes directly.

@chingor13
Copy link
Collaborator Author

I'm not sold on trying to shoehorn autovalue in here. If we were starting from scratch, it would make a lot of sense. We cannot switch many of these implementations over to autovalue as they already have public constructors (including the CommonGoogleClientRequestInitializer)

@chingor13 chingor13 merged commit 6000d9f into googleapis:master Jun 18, 2019
@chingor13 chingor13 deleted the system-parameters branch June 18, 2019 15:56
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. 🚨 This issue needs some love.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants