Conversation
jiasli
requested review from
00Kai0,
changlong-liu,
jsntcy and
msyyc
as code owners
| per_call_policies = kwargs.get('per_call_policies', []) | ||
| if isinstance(per_call_policies, Iterable): | ||
| per_call_policies.append(ARMAutoResourceProviderRegistrationPolicy()) | ||
| per_call_policies = list(per_call_policies).append(ARMAutoResourceProviderRegistrationPolicy()) |
There was a problem hiding this comment.
Fix #18238 (comment)
append will fail if per_call_policies is a tuple, instead of a list:
AttributeError: 'tuple' object has no attribute 'append'
Convert to per_call_policies to a list first.
| config = kwargs.get('config') | ||
| if not config.http_logging_policy: | ||
| config.http_logging_policy = kwargs.get('http_logging_policy', ARMHttpLoggingPolicy(**kwargs)) | ||
| config.http_logging_policy = ARMHttpLoggingPolicy(**kwargs) |
There was a problem hiding this comment.
Fix #18238 (comment)
We should forbid the usage of setting http_logging_policy with kwargs here, because it
- causes duplication with SDK's code
- allows 2 ways to set
http_logging_policy(with eitherkwargsorconfig), which can easily leads to inconsistency
There was a problem hiding this comment.
We have a clear priority order which is config > kwargs > default one.
I would suggest we keeping it because we want to align the priority with others.
There was a problem hiding this comment.
However, in specific client like SubscriptionClient, http_logging_policy in kwargs is firstly used to set self._config.http_logging_policy:
Then it is used as the http_logging_policy attribute of the PipelineClient:
So the priority/flow here is more like kwargs > config > default one.
Also, since PipelineClient doesn't take any policies from kwargs, why don't we remove the support for passing a http_logging_policy via kwargs to ARMPipelineClient as well, just to keep ARMPipelineClient simple (only config is supported)? 🤔
There was a problem hiding this comment.
In auto generated code (90% of the case), we will generate config which accept http_logging_policy. So http_logging_policy in kwargs works in most of the case except the one in azure-core.
ghost
added
the
no-recent-activity
|
Hi @jiasli. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days. |
ghost
closed this
|
Hi @jiasli. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing "/reopen" if you'd like to continue working on these changes. Please be sure to use the command to reopen or remove the "no-recent-activity" label; otherwise, this is likely to be closed again with the next cleanup pass. |
2 participants
Refine #18238's changes on
ARMPipelineClient.__init__