Skip to content

[Core] Config not required for pipeline client #6972

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.

Sign up for GitHub

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

Closed
annatisch wants to merge 4 commits into from
Closed

[Core] Config not required for pipeline client #6972

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bb56cd1
[Core] Config not required for pipeline client
annatisch Aug 28, 2019
68cec05
Updated async pipeline client
annatisch Aug 28, 2019
7425b4c
Reordered default async pipeline
annatisch Aug 28, 2019
3153a73
Reordered default pipeline
annatisch Aug 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions sdk/core/azure-core/azure/core/pipeline_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ class PipelineClient(PipelineClientBase):
Builds a Pipeline client.

:param str base_url: URL for the request.
:param config: Service configuration. This is a required parameter.
:param config: Service configuration. This is a required unless pipeline or policies are given separately.
Copy link
Member

@xiangyan99 xiangyan99 Sep 3, 2019

Choose a reason for hiding this comment

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

Missing "parameter"?

:type config: ~azure.core.Configuration
:param kwargs: keyword arguments
:return: A pipeline object.
Expand All @@ -80,10 +80,8 @@ class PipelineClient(PipelineClientBase):
:caption: Builds the pipeline client.
"""

def __init__(self, base_url, config, **kwargs):
def __init__(self, base_url, config=None, **kwargs):
Copy link
Member

@xiangyan99 xiangyan99 Sep 3, 2019

Choose a reason for hiding this comment

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

Should we put config into kwargs?

super(PipelineClient, self).__init__(base_url)
if config is None:
raise ValueError("Config is a required parameter")
self._config = config
self._base_url = base_url
if kwargs.get("pipeline"):
Expand All @@ -106,6 +104,8 @@ def _build_pipeline(self, config, **kwargs): # pylint: disable=no-self-use
policies = kwargs.get('policies')

if policies is None: # [] is a valid policy list
if config is None:
raise ValueError("No config or policies supplied.")
Copy link
Member

@xiangyan99 xiangyan99 Sep 3, 2019

Choose a reason for hiding this comment

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

Why not if config is None, give a default one?

policies = [
config.headers_policy,
config.user_agent_policy,
Expand All @@ -114,8 +114,8 @@ def _build_pipeline(self, config, **kwargs): # pylint: disable=no-self-use
config.redirect_policy,
config.retry_policy,
config.custom_hook_policy,
config.logging_policy,
DistributedTracingPolicy(),
config.logging_policy,
Copy link
Member

@xiangyan99 xiangyan99 Sep 3, 2019

Choose a reason for hiding this comment

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

Maybe we should add proxy policy as well.

]

if not transport:
Expand Down
10 changes: 5 additions & 5 deletions sdk/core/azure-core/azure/core/pipeline_client_async.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ class AsyncPipelineClient(PipelineClientBase):
Builds an AsyncPipeline client.

:param str base_url: URL for the request.
:param config: Service configuration. This is a required parameter.
:param config: Service configuration. This is a required unless pipeline or policies are given separately.
:type config: ~azure.core.Configuration
:param kwargs: keyword arguments.
:return: An async pipeline object.
Expand All @@ -79,10 +79,8 @@ class AsyncPipelineClient(PipelineClientBase):
:caption: Builds the async pipeline client.
"""

def __init__(self, base_url, config, **kwargs):
def __init__(self, base_url, config=None, **kwargs):
super(AsyncPipelineClient, self).__init__(base_url)
if config is None:
raise ValueError("Config is a required parameter")
self._config = config
self._base_url = base_url
if kwargs.get("pipeline"):
Expand All @@ -105,6 +103,8 @@ def _build_pipeline(self, config, **kwargs): # pylint: disable=no-self-use
policies = kwargs.get('policies')

if policies is None: # [] is a valid policy list
if config is None:
raise ValueError("No config or policies supplied.")
policies = [
config.headers_policy,
config.user_agent_policy,
Expand All @@ -113,8 +113,8 @@ def _build_pipeline(self, config, **kwargs): # pylint: disable=no-self-use
config.redirect_policy,
config.retry_policy,
config.custom_hook_policy,
config.logging_policy,
DistributedTracingPolicy(),
config.logging_policy,
]

if not transport:
Expand Down