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.

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

[Voice] - support for the whole API #27

Merged
merged 53 commits into from
May 10, 2024
Merged

[Voice] - support for the whole API #27

merged 53 commits into from
May 10, 2024

Conversation

650elx
Copy link
Collaborator

@650elx 650elx commented May 6, 2024

@650elx 650elx marked this pull request as ready for review May 10, 2024 09:57
Copy link

@Dovchik Dovchik left a comment

Choose a reason for hiding this comment

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

Good job on PR!
We have here a discrepancy of types accepted by api function and a models, I think it would be nice to unify them to reduce confusion.
Also, the test just checking the instance, it would be nice to have check response field values as well for a more robustness.

@@ -32,6 +32,9 @@ def __init__(
self.auth_origin = "auth.sinch.com"
self.numbers_origin = "numbers.api.sinch.com"
self.verification_origin = "verification.api.sinch.com"
self.voice_applications_origin = "callingapi.sinch.com"
Copy link

Choose a reason for hiding this comment

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

I think it's not used, and overrieded below, but it would be a dot after calling

Suggested change
self.voice_applications_origin = "callingapi.sinch.com"
self.voice_applications_origin = "calling.api.sinch.com"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://developers.sinch.com/docs/voice/api-reference/voice/tag/Applications/
I think there is a separate endpoint for Applications:
url = "https://callingapi.sinch.com/v1/configuration/numbers"

sinch/domains/voice/models/__init__.py Show resolved Hide resolved
sinch/domains/voice/__init__.py Show resolved Hide resolved
sinch/domains/voice/__init__.py Show resolved Hide resolved
sinch/domains/voice/__init__.py Show resolved Hide resolved
@650elx
Copy link
Collaborator Author

650elx commented May 10, 2024

Good job on PR! We have here a discrepancy of types accepted by api function and a models, I think it would be nice to unify them to reduce confusion. Also, the test just checking the instance, it would be nice to have check response field values as well for a more robustness.

Thanks! When it comes to the isinstance thing within tests, the validation happens on the Wiremock level. All discrepancies are then passed to the PyTest and displayed at the end of the test run.

@650elx
Copy link
Collaborator Author

650elx commented May 10, 2024

For the loose enum handling (aligned with our ADR) I have an investigation ticket prepared:
https://tickets.sinch.com/browse/DEVEXP-423

Copy link

@Dovchik Dovchik left a comment

Choose a reason for hiding this comment

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

Approving!
Besides that we decided to investigate a new approach for enums in the future

@650elx 650elx merged commit cdeabc6 into main May 10, 2024
4 checks passed
@650elx 650elx deleted the DEVEXP-306 branch August 22, 2024 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants