[Voice] - conferences and applications #20
Conversation
650elx
requested review from
Dovchik,
krogers0607,
asein-sinch and
JPPortier
as code owners
| dtmf=dtmf, | ||
| custom=custom, | ||
| maxDuration=max_duration, | ||
| ice=ice, |
There was a problem hiding this comment.
The PR #18 says explicitly "This PR omits any SVAML related code. SVAML will be implemented within DEVEXP-306" which is this PR if I'm not mistaken.
I may have missed it but I can't see any support for SVAML here, can you point me to the right place in the code? Or maybe you forgot to push a commit?
There was a problem hiding this comment.
I've simply overlooked it :P it's added now
There was a problem hiding this comment.
Can you then update the test to reflect it? From what I can see, the property is still defined as a string with no hints to build actions and instructions
There was a problem hiding this comment.
I think I've forgot to mention ADR-0005-enums.md in this PR review.
It was a design decision that was made while I was designing this SDK.
That's why I'm providing Enums, helpers etc. (they are present and explained in documentation and code samples) but I don't enforce them.
Maybe we need to revisit this ADR? Until then I prefer to stick to it.
There was a problem hiding this comment.
I may have missed it, but since you are mentioning helpers, can you point me where are the SVAML helpers to build actions and instructions for each kind of events? I would like to compare it with what is done in the Node.js SDK
There was a problem hiding this comment.
My last reply was more abstract (just like mentioned ADR is). It't not about SVAML but the whole SDK design.
There was a problem hiding this comment.
Thanks for the clarification. Now, back to the original comment regarding SVAML, do you plan to add helpers or will you leave it as is?
There was a problem hiding this comment.
IMO current solution is fairly simple to use. I don't think there is a need to add more to that.
There was a problem hiding this comment.
Why a step back about providing SVAML as expected by ticket ?
Seems we need to synch about design and SDKs expectations
| conference_id: str | ||
| call_id: str | ||
| command: str | ||
| moh: str |
There was a problem hiding this comment.
Currently, the moh parameter is specified as a string. While Python allows for flexibility in function parameters, I'd like to highlight the benefits of specifying the type of moh as ConferenceMusicOnHold:
- Type Safety: Specifying the type of
mohasConferenceMusicOnHoldensures that developers use only valid values defined in theConferenceMusicOnHoldenum. This helps catch errors early in development and provides better type checking. - Self-Documenting Code: By using the enum type, we provide clear documentation for developers who will use the function in the future. It communicates the expected input and improves code readability.
- Flexibility: Despite specifying the type, developers still have the flexibility to pass a string directly to
mohif they prefer. For example, they can usemanage_participant(moh="ring")instead ofmanage_participant(moh=ConferenceMusicOnHold.RING.value)if it better suits their needs. - Consistency: It's worth noting that we already have a similar example in our codebase where enum types are used to enforce strict input validation. This change would help maintain consistency across our codebase and make it easier for developers to understand and work with our APIs.
I believe incorporating this change will enhance the clarity, robustness, and maintainability of our codebase. What are your thoughts on making this improvement?
|
This PR is redundant. There will be new PR for Voice API (as we discussed) |
I've changed target branch to
main, so this PR represents whole Voice domain.