Azure Communication Services - Refactor PhoneNumberAsyncClient to Wrap Exceptions#15794
Conversation
waynemo
requested review from
JianpingChen and
RezaJooyandeh
as code owners
waynemo
requested review from
ankitarorabit,
jbeauregardb,
paolamvhz and
srnagar
| @@ -80,10 +94,8 @@ PagedFlux<AcquiredPhoneNumber> listAllPhoneNumbers(String locale, Context contex | |||
| @ServiceMethod(returns = ReturnType.SINGLE) | |||
| public Mono<AreaCodes> getAllAreaCodes( | |||
There was a problem hiding this comment.
Not related to the immediate change, but wondering why these methods are not following the 'list' paradigm, like listAreaCodes, listAreaCodesWithResponse etc.?
There was a problem hiding this comment.
The main difference here the return value. AreaCodes is a composite object that contains data. It is not a pageable list in this case, so we are not using the list* convention.
To avoid confusion, perhaps we can rename it to "getAreaCodes()" instead in a separate PR.
| locationOptionsQueries.setLocationOptions(locationOptions); | ||
| return phoneNumberAdministrations.getAllAreaCodesAsync( | ||
| locationType, countryCode, phonePlanId, locationOptionsQueries); | ||
| return getAllAreaCodesWithResponse(locationType, countryCode, phonePlanId, locationOptions) |
There was a problem hiding this comment.
Isn't it better to invoke the regular (without response) method and wrap in an exception.. this process of invoking getAllAreaCodesWithResponse and then trying to flatten doesn't look very clean?
There was a problem hiding this comment.
We can get the Entity model from the *WithResponse method, but we cannot get the Response information from the regular method (without response). So if we want to reuse the methods we need to start from the *WithResponse method.
| @@ -61,11 +67,19 @@ public final class PhoneNumberAsyncClient { | |||
| */ | |||
| @ServiceMethod(returns = ReturnType.COLLECTION) | |||
| public PagedFlux<AcquiredPhoneNumber> listAllPhoneNumbers(String locale) { | |||
There was a problem hiding this comment.
Small clarification, based on the guidelines shouldn't it be listPhoneNumbers? Maybe 'list' and 'listAll' are both OK, just trying to get some understanding here, this is what I see in the doc..
YOU SHOULD remain flexible and use names best suited for developer experience. Don’t let the naming rules result in non-idiomatic naming patterns. For example, Java developers prefer list operations over getAll operations.
There was a problem hiding this comment.
I think the guidelines are more concerned with the 'list' vs 'get' prefix. So both 'list' and 'listAll' will work to differentiate from 'get'.
The potential improvement might be to remove 'All' since it is a bit redundant. Let's revisit that separately.
…p Exceptions (Azure#15794) * Catch exceptions and return error Mono/PagedFlux * Refactor PhoneNumberAsyncClient method overloads to consolidate logic
…p Exceptions (Azure#15794) * Catch exceptions and return error Mono/PagedFlux * Refactor PhoneNumberAsyncClient method overloads to consolidate logic
* Azure Communication Service - Phone Number Administration SDK (#15497) * Add Phone Number functionality to azure-communication-administration * Update checkstyle suppressions * Azure Communication Services - Refactor PhoneNumberAsyncClient to Wrap Exceptions (#15794) * Catch exceptions and return error Mono/PagedFlux * Refactor PhoneNumberAsyncClient method overloads to consolidate logic * Azure Communication Service - Update Phone Number Admin Null Checks (#15811) * Add null checks for required method arguments * Remove setter null check to allow clearing of httpPipeline * Updating azure-communication-administration README with a few samples (#15816) * Updating javadoc with a few samples * resolve comments * Fixing spot check errors * fix typo * Add Env Var check to skip PhoneNumber integration tests (#15855) * Samples and Readme update for Administration package (#15853) * More samples and readme changes * PR comments * build failure * fix indentation * azure-communication-administration - Regenerate Code for Updated Swagger (#15861) * Update swagger and regenerate code * Fix tests after code regeneration * Fix the param name * updating the sample code Co-authored-by: waynemo <wamo@microsoft.com>
2 participants
Refactor methods in PhoneNumberAsyncClient to ensure methods return Mono.error or Flux.error instead of throwing exceptions.
The changes address the comment here:
#15497 (comment)