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.

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

Jump to bottom

[Communication] - Phone Number - Redesigned API #16671

Merged
merged 32 commits into from
Mar 6, 2021
Merged

[Communication] - Phone Number - Redesigned API #16671

merged 32 commits into from
Mar 6, 2021

Conversation

jbeauregardb
Copy link
Contributor

@jbeauregardb jbeauregardb commented Feb 10, 2021 •
edited
Loading

This PR has the new redesign for the PNM package.

  • The code was regenerated using the new swagger file
  • Both, sync and async clients were rewritten from the ground up
  • Tests were also rewritten from the ground up
  • README and Changelog were modified to document the new functions
  • New samples were created to match with the new implementation

@ghost ghost added the Communication label Feb 10, 2021
annatisch
annatisch previously requested changes Feb 15, 2021
View reviewed changes
Copy link
Member

@annatisch annatisch left a comment

Choose a reason for hiding this comment

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

Could we please get an updated APIview for these changes? :)

...n/azure-communication-phonenumbers/azure/communication/phonenumbers/_phone_numbers_client.py Show resolved Hide resolved
sdk/communication/azure-communication-phonenumbers/azure/communication/phonenumbers/__init__.py Outdated Show resolved Hide resolved
...n/azure-communication-phonenumbers/azure/communication/phonenumbers/_phone_numbers_client.py Outdated Show resolved Hide resolved
...n/azure-communication-phonenumbers/azure/communication/phonenumbers/_phone_numbers_client.py Outdated Show resolved Hide resolved
...n/azure-communication-phonenumbers/azure/communication/phonenumbers/_phone_numbers_client.py Outdated Show resolved Hide resolved
sdk/communication/azure-communication-phonenumbers/samples/get_phone_number_sample.py Outdated Show resolved Hide resolved
...communication/azure-communication-phonenumbers/samples/list_acquired_phone_numbers_sample.py Outdated Show resolved Hide resolved
turalf
turalf reviewed Feb 19, 2021
View reviewed changes
Copy link
Contributor

@turalf turalf left a comment

Choose a reason for hiding this comment

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

Please remove whl file

sdk/communication/azure-communication-phonenumbers/samples/get_phone_number_sample.py Outdated Show resolved Hide resolved
...nenumbers/test/recordings/test_phone_number_administration_client.test_get_phone_number.yaml Outdated Show resolved Hide resolved
...cordings/test_phone_number_administration_client_async.test_list_acquired_phone_numbers.yaml Outdated Show resolved Hide resolved
...test/recordings/test_phone_number_administration_client_async.test_release_phone_number.yaml Outdated Show resolved Hide resolved
sdk/communication/azure-communication-phonenumbers/setup.cfg Outdated Show resolved Hide resolved
...n/azure-communication-phonenumbers/azure/communication/phonenumbers/_phone_numbers_client.py Outdated Show resolved Hide resolved
beltr0n
beltr0n requested changes Mar 5, 2021
View reviewed changes
sdk/communication/azure-communication-phonenumbers/README.md Outdated Show resolved Hide resolved
sdk/communication/azure-communication-phonenumbers/README.md Show resolved Hide resolved
sdk/communication/azure-communication-phonenumbers/README.md Outdated Show resolved Hide resolved
sdk/communication/azure-communication-phonenumbers/README.md Outdated Show resolved Hide resolved
sdk/communication/azure-communication-phonenumbers/README.md Outdated Show resolved Hide resolved
...munication/azure-communication-phonenumbers/samples/search_available_phone_numbers_sample.py Outdated Show resolved Hide resolved
...nication/azure-communication-phonenumbers/samples/update_phone_number_capabilities_sample.py Outdated Show resolved Hide resolved
sdk/communication/azure-communication-phonenumbers/setup.py Show resolved Hide resolved
sdk/communication/azure-communication-phonenumbers/test/_shared/testcase.py Outdated Show resolved Hide resolved
...one_number_administration_client.test_list_acquired_phone_numbers_from_managed_identity.yaml Show resolved Hide resolved
sacheu
sacheu reviewed Mar 5, 2021
View reviewed changes
...n/azure-communication-phonenumbers/azure/communication/phonenumbers/_phone_numbers_client.py
from azure.core.tracing.decorator import distributed_trace
from azure.core.paging import ItemPaged
from azure.core.polling import LROPoller
from ._generated._phone_numbers_client import PhoneNumbersClient as PhoneNumbersClientGen
Copy link
Member

Choose a reason for hiding this comment

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

Is this common practice to have import class from generated code as XXXGen?

sacheu
sacheu reviewed Mar 5, 2021
View reviewed changes
...mmunication-phonenumbers/azure/communication/phonenumbers/aio/_phone_numbers_client_async.py
from azure.core.tracing.decorator_async import distributed_trace_async
from azure.core.paging import ItemPaged
from azure.core.polling import LROPoller
from .._generated.aio._phone_numbers_client import PhoneNumbersClient as PhoneNumbersClientGen
Copy link
Member

Choose a reason for hiding this comment

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

Is 'import {A class} as {A Class}Gen' a common practice? why we need to do this?
I don't see next line is doing the same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's also done in the Identity client and it was also done in the previous implementation of this PNM SDK, so I just left it as it was

@sacheu
Copy link
Member

sacheu commented Mar 5, 2021

Do we need to re-enable LIve TEST for phone number in this PR? I don't see that change?
And I see you checkin a binary file, a, wheel file. Is that common practice?

@sacheu
Copy link
Member

sacheu commented Mar 5, 2021

No need to checkin ...on/azure-communication-identity/azure_communication_identity-1.0.0b4-py2.py3-none-any.whl since it is empty file.

@jbeauregardb
Copy link
Contributor Author

/azp run python - communication - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sacheu sacheu requested review from sacheu and beltr0n and removed request for sacheu March 5, 2021 20:17
sacheu
sacheu approved these changes Mar 5, 2021
View reviewed changes
Copy link
Member

@sacheu sacheu left a comment

Choose a reason for hiding this comment

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

Looks Good. Thanks Jorge.

@jbeauregardb jbeauregardb force-pushed the pnm-redesign branch 2 times, most recently from 6fa85e3 to 471f511 Compare March 5, 2021 21:18
@jbeauregardb
Copy link
Contributor Author

/azp run python - communication - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@annatisch annatisch dismissed their stale review March 5, 2021 21:57

Thanks!

@jbeauregardb jbeauregardb reopened this Mar 5, 2021
@jbeauregardb jbeauregardb merged commit 5f0f752 into Azure:master Mar 6, 2021
iscai-msft added a commit that referenced this pull request Mar 8, 2021
@iscai-msft
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-python …
2b7ce35 
…into update_ta_tests

* 'master' of https://github.com/Azure/azure-sdk-for-python:
  Update get_package_properties.py logic for python 2.7 (#17144)
  update changelog (#17150)
  [ServiceBus] 7.1.0 Release update changelog (#17135)
  [ServiceBus] Object mapping support (#17080)
  move SetTestPipeline into its own template (#17141)
  Revise token cache configuration API (#16326)
  Fix dup cloud error (#17097)
  Perf tests for monitor exporter (#17067)
  [Communication] - Phone Number - Redesigned API (#16671)
  disable retry (#17078)
  [Key Vault] Add perf tests for certificates, keys, and secrets (#17073)
  [text analytics] Analyze updates for v5.1.0b6 (#17003)
  Add any additional claims to AuthenticationRequiredError (#17136)
  Fix logic in SetTestPipelineVersionInEngCommon (#17138)
  [Key Vault] Make test resource cleanup script asynchronous (#17032)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@turalf turalf turalf left review comments

@sacheu sacheu sacheu approved these changes

@beltr0n beltr0n beltr0n approved these changes

@annatisch annatisch annatisch left review comments

@ankitarorabit ankitarorabit Awaiting requested review from ankitarorabit

@memontic-ms memontic-ms Awaiting requested review from memontic-ms

@00Kai0 00Kai0 Awaiting requested review from 00Kai0

@abhahn abhahn Awaiting requested review from abhahn

@amishra-dev amishra-dev Awaiting requested review from amishra-dev

@catalinaperalta catalinaperalta Awaiting requested review from catalinaperalta

@changlong-liu changlong-liu Awaiting requested review from changlong-liu

@chlowell chlowell Awaiting requested review from chlowell

@cRui861 cRui861 Awaiting requested review from cRui861

@danieljurek danieljurek Awaiting requested review from danieljurek

@dpwatrous dpwatrous Awaiting requested review from dpwatrous

@giakas giakas Awaiting requested review from giakas

@gingi gingi Awaiting requested review from gingi

@iscai-msft iscai-msft Awaiting requested review from iscai-msft

@johanste johanste Awaiting requested review from johanste

@jsntcy jsntcy Awaiting requested review from jsntcy

@kasobol-msft kasobol-msft Awaiting requested review from kasobol-msft

@kristapratico kristapratico Awaiting requested review from kristapratico

@lmazuel lmazuel Awaiting requested review from lmazuel

@lzchen lzchen Awaiting requested review from lzchen

@mayurid mayurid Awaiting requested review from mayurid

@mccoyp mccoyp Awaiting requested review from mccoyp

@mitchdenny mitchdenny Awaiting requested review from mitchdenny

@msyyc msyyc Awaiting requested review from msyyc

@paterasMSFT paterasMSFT Awaiting requested review from paterasMSFT

@rakshith91 rakshith91 Awaiting requested review from rakshith91

@scbedd scbedd Awaiting requested review from scbedd

@schaabs schaabs Awaiting requested review from schaabs

@seankane-msft seankane-msft Awaiting requested review from seankane-msft

@southpolesteve southpolesteve Awaiting requested review from southpolesteve

@swathipil swathipil Awaiting requested review from swathipil

@tasherif-msft tasherif-msft Awaiting requested review from tasherif-msft

@weshaggard weshaggard Awaiting requested review from weshaggard

@xiafu-msft xiafu-msft Awaiting requested review from xiafu-msft

@xiangyan99 xiangyan99 Awaiting requested review from xiangyan99

@yunhaoling yunhaoling Awaiting requested review from yunhaoling

@zezha-msft zezha-msft Awaiting requested review from zezha-msft

@zfengms zfengms Awaiting requested review from zfengms

@zfoster zfoster Awaiting requested review from zfoster

Assignees
No one assigned
Labels
Communication
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jbeauregardb @sacheu @beltr0n @turalf @annatisch @jorge-beauregard