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
design, validate parameters in Builder #39889
base: main
Are you sure you want to change the base?
design, validate parameters in Builder #39889
Conversation
@@ -283,8 +284,17 @@ private ContentSafetyClientImpl buildInnerClient() { | |||
return client; | |||
} | |||
|
|||
@Generated | |||
private void validateBuilderParameters() { | |||
Objects.requireNonNull(endpoint, "'endpoint' cannot be null."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change on endpoint
would apply to any required client properties.
discussion
- Should we use a different exception, e.g.
InvalidStateException
? - Do we put these "default validation" here, or in
createHttpPipeline
(so that the whole method is generated as empty and intended for customization)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
InvalidStateException
seems most correct to me.
I think I'm in favor of leaving all of the validation in this method. Perhaps we generate a comment that these are the default requirements and others can be added? "Magic" other validation seems hard to understand.
API change check API changes are not detected in this pull request. |
@Generated | ||
private HttpPipeline createHttpPipeline() { | ||
validateBuilderParameters(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why here and not in buildInnerClient
? It doesn't matter much in this example; are there others where it might matter more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative code flow would be user setting the pipeline
via
Line 88 in 020a2a6
public ContentSafetyClientBuilder pipeline(HttpPipeline pipeline) { |
And builderInnerClient
has this
Lines 278 to 279 in 020a2a6
private ContentSafetyClientImpl buildInnerClient() { | |
HttpPipeline localPipeline = (pipeline != null) ? pipeline : createHttpPipeline(); |
(
createHttpPipeline
won't be called, if user provides the pipeline
)
I think in that code flow, there is no need to verify anything (as user provides a pipeline and we cannot inspect it anyway).
Description
Please add an informative description that covers that changes made by the pull request and link all relevant issues.
If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines