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

sample.py lists all the results found and takes credentials from a co… #283

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

arash2060
Copy link

…nfiguration file

Copy link
Contributor

@watterso watterso left a comment

Choose a reason for hiding this comment

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

Appreciate the contribution!

I like the addition of support of the config file, the other changes distract from the main purpose of this file - to be a basic example program. I made comments on those lines, could you undo the changes?

GRANT_TYPE = 'client_credentials'


# Defaults for our simple example.
DEFAULT_TERM = 'dinner'
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we leave the term being a generic search? Makes it more relatable

CLIENT_SECRET = None

config = configparser.RawConfigParser()
config.read('API_Keys.cfg')
Copy link
Contributor

Choose a reason for hiding this comment

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

could you include a default API_Keys.cfg with bogus values?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also please don't forget to update it to use an API_KEY rather than CLIENT_ID / CLIENT_SECRET, more details here

@@ -205,3 +215,6 @@ def main():

if __name__ == '__main__':
main()

Copy link
Contributor

Choose a reason for hiding this comment

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

The comment below isn't providing much, could you delete it please

@@ -174,10 +181,13 @@ def query_api(term, location):
print(u'{0} businesses found, querying business info ' \
'for the top result "{1}" ...'.format(
len(businesses), business_id))
response = get_business(bearer_token, business_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you undo the changes to query_api ? This is supposed to be a very basic example program, just printing the top result keeps the example simple and easy to read, adding more complicated logic distracts from the functionality it is trying to highlight

@loganibo loganibo deleted the branch Yelp:master April 3, 2023 17:50
@loganibo loganibo deleted the master branch April 3, 2023 17:50
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