-
Notifications
You must be signed in to change notification settings - Fork 139
Implemented Regional API URL in config instead of hardcoded #32
Conversation
|
@sluongng, It will cover your contributions to all Microsoft-managed open source projects. |
|
@sluongng, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request. |
|
|
||
| import cognitive_face as CF | ||
|
|
||
| _BASE_URL = 'https://westus.api.cognitive.microsoft.com/face/v1.0/' |
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.
Instead of setting/getting Region, what about making it more general to set/get BaseUrl directly?
cognitive_face/util.py
Outdated
| class BaseUrl(object): | ||
|
|
||
| @classmethod | ||
| def set(cls, url): |
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.
Prefer to use base_url for consistency.
|
Hi @sluongng, really appreciate for what you have done. Just some minor suggestions wish you can consider in your pull request. |
|
I have modified my solution as requested. Feedbacks are more than welcome. |
|
Thanks for your contribution! |
What did you implement:
Addressed #31 and #29
How did you implement it:
https://westus.api.cognitive.microsoft.com/face/v1.0/to avoid any disruption to existing code bases./cognitive_face/tests/config.sample.pyHow can we verify it:
I have modified the test suit appropriately.
Just run
python setup.py testTodos:
Is this ready for review?: YES
Is it a breaking change?: NO