Skip to content
This repository was archived by the owner on Nov 8, 2021. It is now read-only.

Conversation

@sluongng
Copy link
Contributor

@sluongng sluongng commented May 17, 2017

What did you implement:

Addressed #31 and #29

How did you implement it:

  • Created new class: BaseUrl following the Key class template
  • BaseUrl will use BaseUrl.get() and BaseUrl was exposed and totally configurable.
  • Default base url is set to https://westus.api.cognitive.microsoft.com/face/v1.0/ to avoid any disruption to existing code bases.
  • Added BASE_URL config template to /cognitive_face/tests/config.sample.py
  • Fixed some existing typos

How can we verify it:

I have modified the test suit appropriately.

Just run python setup.py test

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config/commands/resources
  • Change ready for review message below

Is this ready for review?: YES

Is it a breaking change?: NO

@msftclas
Copy link

@sluongng,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla.microsoft.com.

It will cover your contributions to all Microsoft-managed open source projects.
Thanks,
Microsoft Pull Request Bot

@msftclas
Copy link

@sluongng, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, Microsoft Pull Request Bot


import cognitive_face as CF

_BASE_URL = 'https://westus.api.cognitive.microsoft.com/face/v1.0/'
Copy link
Contributor

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?

class BaseUrl(object):

@classmethod
def set(cls, url):
Copy link
Contributor

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.

@huxuan
Copy link
Contributor

huxuan commented May 17, 2017

Hi @sluongng, really appreciate for what you have done. Just some minor suggestions wish you can consider in your pull request.

@sluongng
Copy link
Contributor Author

I have modified my solution as requested. Feedbacks are more than welcome.

@huxuan huxuan merged commit 7a5862a into microsoft:master May 17, 2017
@huxuan
Copy link
Contributor

huxuan commented May 17, 2017

Thanks for your contribution!

YaseenAlk pushed a commit to YaseenAlk/Cognitive-Face-Python that referenced this pull request Aug 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants