-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[Container Registry] ACR list_repositories pageable #17714
Conversation
/azp run python - containerregistry - ci |
Azure Pipelines successfully started running 1 pipeline(s). |
error_map.update(kwargs.pop("error_map", {})) | ||
accept = "application/json" | ||
|
||
def prepare_request(next_link=None): |
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.
If this method is the same as the sync prepare_request
, would it be possible to place this in a shared base class?
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.
It probably is, this all had to be copied from the generated code. I'll open an issue to get this done over the next sprint. Thanks for the rec. #17817
error_map.update(kwargs.pop("error_map", {})) | ||
accept = "application/json" | ||
|
||
def prepare_request(next_link=None): |
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.
Same comment, about potentially sharing code
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.
I don't know the story behind the gen code copy, but assume there is an issue open for it? LGTM
...tainerregistry/azure-containerregistry/azure/containerregistry/_container_registry_client.py
Outdated
Show resolved
Hide resolved
...tainerregistry/azure-containerregistry/azure/containerregistry/_container_registry_client.py
Outdated
Show resolved
Hide resolved
...inerregistry/azure-containerregistry/azure/containerregistry/_container_repository_client.py
Outdated
Show resolved
Hide resolved
...inerregistry/azure-containerregistry/azure/containerregistry/_container_repository_client.py
Outdated
Show resolved
Hide resolved
...inerregistry/azure/containerregistry/_generated/operations/_container_registry_operations.py
Show resolved
Hide resolved
@@ -24,15 +32,15 @@ | |||
|
|||
|
|||
class ContainerRepositoryClient(ContainerRegistryBaseClient): | |||
def __init__(self, endpoint: str, repository: str, credential: "AsyncTokenCredential", **kwargs) -> None: | |||
def __init__(self, endpoint: str, repository: str, credential: "AsyncTokenCredential", **kwargs: Dict[str, Any]) -> None: |
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.
I don't think Dict[str, Any]
is wrong for **kwargs, but I think I've generally seen this documented as **Any
in other libraries.
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.
looks like Krista left the small nits that I was going to nit, so deleted them, but o/w LGTM
@kristapratico yes #17817 . The gen code copy had to happen because the link parsing isn't supported by the current autorest, but is in the works. |
/check-enforcer evaluate |
Adding xms ids for Blockchain (Azure#17714) * Adding xms-ids for Blockchain * Update readme.md * Update readme.python.md Co-authored-by: Yuchao Yan <[email protected]>
No description provided.