[CommunicationIdentifier] Introduce new rawId and creation of identifer #1255
Conversation
ghost
added
the
JoshuaLai
requested review from
DominikMe,
petrsvihlik,
waynemo,
tjprescott,
mpodwysocki,
prprabhu-ms and
RezaJooyandeh
sdk/communication/AzureCommunicationCommon/Source/CommunicationIdentifierHelper.swift
Outdated
Show resolved
Hide resolved
sdk/communication/AzureCommunicationCommon/Source/Identifiers.swift
Outdated
Show resolved
Hide resolved
sdk/communication/AzureCommunicationCommon/Tests/CommunicationIdentifierHelperTests.swift
Outdated
Show resolved
Hide resolved
sdk/communication/AzureCommunicationCommon/Tests/CommunicationIdentifierHelperTests.swift
Outdated
Show resolved
Hide resolved
sdk/communication/AzureCommunicationCommon/Tests/CommunicationIdentifierHelperTests.swift
Show resolved
Hide resolved
| Helper class to easily create CommunicationIdentifiers | ||
| */ | ||
| @objcMembers | ||
| public class CommunicationIdentifierHelper: NSObject { |
There was a problem hiding this comment.
I'm never a fan of anything that ends in Helper, Util or Manager :-).
@tjprescott Any suggestions for good Swift-idiomatic naming?
Ideally we would have liked a static factory method on CommunicationIdentifier itself but can't do it because it's a protocol. Another alternative is a free function.
There was a problem hiding this comment.
Why not add a required init to the protocol that takes a rawId and then provide the contents of createCommunicationIdentifier(from:) as a default implementation via extension?
There was a problem hiding this comment.
But I agree you don't really want this helper thingy as part of the public API.
There was a problem hiding this comment.
That was something I was trying. I initially was thinking something like this
@objc public protocol CommunicationIdentifier: NSObjectProtocol {
@objc var rawId: String { get }
static func createCommunicationIdentifier(from rawId: String) -> CommunicationIdentifier
}
extension CommunicationIdentifier {
public static func createCommunicationIdentifier(from rawId: String) -> CommunicationIdentifier {
}
}
But because the CommunicationIdentifier needed to be exposed to Obj-C I can't create a function inside my extension nicely.
Otherwise I would be forcing the identifiers to all require this method and every method would do the exact same thing.
There was a problem hiding this comment.
@tjprescott @DominikMe Had a discussion about this implementation. I think in terms of functionality this approah that we have with the helper most closely aligns with the other platforms.
That being said, the only reason we are doing this is because of obj-c support. I do think we should slowly start exploring having 2 implementations. One design to favour swift, and this implementation to favour obj-c. We can have in the documentation to guide the developers which approach they should use.
| Helper class to easily create CommunicationIdentifiers | ||
| */ | ||
| @objcMembers | ||
| public class CommunicationIdentifierHelper: NSObject { |
There was a problem hiding this comment.
Why not add a required init to the protocol that takes a rawId and then provide the contents of createCommunicationIdentifier(from:) as a default implementation via extension?
| Helper class to easily create CommunicationIdentifiers | ||
| */ | ||
| @objcMembers | ||
| public class CommunicationIdentifierHelper: NSObject { |
There was a problem hiding this comment.
But I agree you don't really want this helper thingy as part of the public API.
| @@ -30,11 +30,15 @@ import Foundation | |||
| Common Communication Identifier protocol for all Azure Communication Services. | |||
| All Communication Identifiers conform to this protocol. | |||
| */ | |||
| @objc public protocol CommunicationIdentifier: NSObjectProtocol {} | |||
| @objc public protocol CommunicationIdentifier: NSObjectProtocol { | |||
| var rawId: String { get } | |||
There was a problem hiding this comment.
You should be able to require:
init?(from rawId: String)
There was a problem hiding this comment.
Another suggestion that was brought up by @tjprescott that I really like is including a new enum or string to represent the kind. @DominikMe I know we have a similar concept in the TS library.
…promote shared behaviour
… with other platforms
|
This repository is protected by Check Enforcer. The check-enforcer check-run will not pass until there is at least one more check-run successfully passing. Check Enforcer supports the following comment commands:
|
|
/check-enforcer evaluate |
|
/check-enforcer override |
| @@ -1,5 +1,13 @@ | |||
| # Release History | |||
|
|
|||
| ## 1.1.0 (upcoming) | |||
| ### New Features | |||
| - Introduce new `CommunicationIdentifierHelper` to create `CommunicationIdentifier` based on rawIds | |||
There was a problem hiding this comment.
Would be nice to have a more detailed explanation of the change, similar to https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/communication/communication-common/CHANGELOG.md#features-added
|
Closing this PR in favour of (#1312) |
The
CommunicationIdentifierdesign provides a good abstraction of Azure Communication Services internal id format with better type safety, auto-complete and hides internal knowledge.Create new
rawIdinCommunicationIdentifierprotocolIntroduce
CommunicationIdentifierHelperhelp deserialize the mri and convenient way to create appropriate identifiersDetails Internal wiki