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

[Tables] Client and pipeline refactor #18045

Merged
merged 16 commits into from
Apr 16, 2021
Merged

Conversation

annatisch
Copy link
Member

No description provided.

@ghost ghost added the Tables label Apr 14, 2021
Comment on lines 56 to 57
async def __aenter__(self):
await self._client.__aenter__()
Copy link
Member

Choose a reason for hiding this comment

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

Because this inherits from the sync base client, will there be a weird behavior where you can use an async client as a sync context manager:

with TableClient(...) as tc:
    await tc.get_entity(...)

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope - the async client does not inherit from the sync base client. For exactly this reason. It only inherits from the AccountHostsMixin

@annatisch
Copy link
Member Author

/azp run python - tables - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@seankane-msft seankane-msft left a comment

Choose a reason for hiding this comment

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

A few small things

Comment on lines +55 to +68
_SUPPORTED_API_VERSIONS = ["2019-02-02", "2019-07-07"]


def get_api_version(kwargs, default):
# type: (Dict[str, Any], str) -> str
api_version = kwargs.pop("api_version", None)
if api_version and api_version not in _SUPPORTED_API_VERSIONS:
versions = "\n".join(_SUPPORTED_API_VERSIONS)
raise ValueError(
"Unsupported API version '{}'. Please select from:\n{}".format(
api_version, versions
)
)
return api_version or default
Copy link
Member

Choose a reason for hiding this comment

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

Do we support multiple API versions now? Are there major differences between the two?

Copy link
Member Author

Choose a reason for hiding this comment

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

One of the guidelines is that all SDKs support an API version parameter. So that part is mandatory. However in next stand up we should confirm what all languages are setting as the default API version.

sdk/tables/azure-data-tables/tests/test_retry.py Outdated Show resolved Hide resolved
sdk/tables/azure-data-tables/tests/test_retry_async.py Outdated Show resolved Hide resolved
annatisch and others added 5 commits April 16, 2021 07:11
Co-authored-by: Sean Kane <68240067+seankane-msft@users.noreply.github.com>
Co-authored-by: Sean Kane <68240067+seankane-msft@users.noreply.github.com>
@annatisch annatisch marked this pull request as ready for review April 16, 2021 15:05
Copy link
Member

@seankane-msft seankane-msft left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

@annatisch annatisch merged commit 6be6d47 into Azure:master Apr 16, 2021
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-python that referenced this pull request Mar 22, 2022
New API version to Microsoft.Security 2021-11-01 (Azure#18045)

* Adds base for updating Microsoft.Security from version stable/2021-01-01 to version 2021-11-01

* Updates readme

* Updates API version in new specs and examples

* New API version to Microsoft.Security

pick 5459ef18a7 New API version to Microsoft.Security
pick c8462f9c60 [Microsoft.Security alerts] fix examples/Alerts/SimulateAlerts_example.json
pick b5f4550d6b [Microsoft.Security alerts] add missing API to readme.md

* [Microsoft.Security alerts] fix examples/Alerts/SimulateAlerts_example.json

* [Microsoft.Security alerts] add missing API to readme.md

* Fix readme.md file after rebase

* Removing "x-ms-long-running-operation" header

* Setting target package back to package-composite-v3

* Updating securityContacts.json to the latest version

* Reverting securityContacts.json to equal main, should be updated by the relevant team

Co-authored-by: Nitsan Bracha <nibracha@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants