-
Notifications
You must be signed in to change notification settings - Fork 1
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
Conversation API support #13
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part one
@@ -3,10 +3,12 @@ import { Sms } from '@sinch/sms'; | |||
import { Verification } from '@sinch/verification'; | |||
import { SinchClientParameters } from '@sinch/sdk-client'; | |||
import { Voice } from '@sinch/voice'; | |||
import { ConversationDomain } from '@sinch/conversation'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent naming with other products: why Domain
added to conversation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because Conversation
is already the name of an exported model
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AH, dunno, the break in consistency in top api level seems to hit me harder than on model level
Isn't it better to rename the model to ConversationModel
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ConversationModel
would then differ from the online documentation.
ConversationDomain
is used only by the SinchClient to initialize the class and should not be used by the end user.
Either way it's not ideal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Branch to be merged on
main
when the official documentation will be delivered