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

[Azure.Communication.Common] - Overridden operators for the CommunicationIdentifier #30075

Merged
merged 10 commits into from
Jul 29, 2022
Merged

[Azure.Communication.Common] - Overridden operators for the CommunicationIdentifier #30075

merged 10 commits into from
Jul 29, 2022

Conversation

petrsvihlik
Copy link
Contributor

@petrsvihlik petrsvihlik commented Jul 22, 2022 •
edited
Loading

@ghost ghost added the Communication label Jul 22, 2022
@petrsvihlik petrsvihlik marked this pull request as ready for review July 22, 2022 07:43
@azure-sdk
Copy link
Collaborator

azure-sdk commented Jul 22, 2022 •
edited
Loading

API change check

APIView has identified API level changes in this PR and created following API reviews.

Azure.Communication.Common
Azure.Communication.Common

DominikMe
DominikMe reviewed Jul 22, 2022
View reviewed changes
Copy link
Member

@DominikMe DominikMe left a comment

Choose a reason for hiding this comment

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

Thanks!
Nits and I have concerns about the implicit cast.

@petrsvihlik petrsvihlik requested a review from DominikMe July 25, 2022 09:02
@DominikMe
Copy link
Member

DominikMe commented Jul 25, 2022 •
edited
Loading

@petrsvihlik I'm okay with the explicit cast. In documentation do you think we should promote to use (CommunicationIdentifier) 8:orgid:cf2d38b4-d46e-48cd-b95d-dbe64d99b99d or CommunicationIdentifier.FromRawId("8:orgid:cf2d38b4-d46e-48cd-b95d-dbe64d99b99d")?

@petrsvihlik petrsvihlik mentioned this pull request Jul 26, 2022
Closed
@petrsvihlik
Copy link
Contributor Author

@petrsvihlik I'm okay with the explicit cast. In documentation do you think we should promote to use (CommunicationIdentifier) 8:orgid:cf2d38b4-d46e-48cd-b95d-dbe64d99b99d or CommunicationIdentifier.FromRawId("8:orgid:cf2d38b4-d46e-48cd-b95d-dbe64d99b99d")?

Depending on how the docs will be structured, I'd try to follow the pattern that we usually apply when we use abbreviations - first we use the full form, and the subsequent occurrences use the shortened form.
In this case, I'd probably add a comment to the first occurrence of the explicit cast, explaining that the FromRawId() method is used behind the scenes. WDYT?

@petrsvihlik
Copy link
Contributor Author

/azp run net - communication - ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@petrsvihlik petrsvihlik merged commit 9713f41 into Azure:main Jul 29, 2022
@petrsvihlik
Copy link
Contributor Author

https://apiview.dev/Assemblies/Review/0d13d0a5ceaa494f8a920f01b59d206d?diffRevisionId=0561b06141784030aa0b3c05529400f4&doc=False&diffOnly=False&revisionId=e5d38220943a4fc5a4a4597f3190b29e

sofiar-msft pushed a commit to sofiar-msft/azure-sdk-for-net that referenced this pull request Dec 7, 2022
@petrsvihlik @sofiar-msft
[Azure.Communication.Common] - Overridden operators for the Communica…
d87e95c 
…tionIdentifier (Azure#30075)

* overridden operators for the CommunicationIdentifier
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DominikMe DominikMe DominikMe approved these changes

@AikoBB AikoBB AikoBB approved these changes

@maximrytych-ms maximrytych-ms maximrytych-ms approved these changes

@KrzysztofCwalina KrzysztofCwalina Awaiting requested review from KrzysztofCwalina

@tg-msft tg-msft Awaiting requested review from tg-msft

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

Successfully merging this pull request may close these issues.
5 participants
@petrsvihlik @azure-sdk @DominikMe @AikoBB @maximrytych-ms