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

Business Match API (closes #6) #7

Merged
merged 5 commits into from
Oct 20, 2017
Merged

Conversation

zolrath
Copy link
Contributor

@zolrath zolrath commented Oct 19, 2017

This adds the Business Match API as requested in #6
I didn't pull request it as the match API didn't seem to be very effective for me in my testing, though it could have simply been the businesses I was trying to match on since the endpoint is beta.

@gfairchild
Copy link
Collaborator

Upon a quick inspection, this looks solid. Would you mind adding a bullet at the bottom of README.md indicating that we now support this endpoint?

@zolrath
Copy link
Contributor Author

zolrath commented Oct 19, 2017

You got it!
The examples in the examples.py file aren't great as I haven't actually gotten a reasonable response out of the API just yet. I tend to get no results with best and two sometimes unrelated results with lookup.

@gfairchild
Copy link
Collaborator

Awesome! I haven't had a chance to actually pull this down and test it yet, but I'll do that shortly (most likely tomorrow). Would you mind making that .get() change? I think that's the last thing I see offhand.

@gfairchild
Copy link
Collaborator

Oh, another 2 small changes:

  1. Would you mind adding an entry to CHANGES.md describing this change? Let's up the version to 2.1.0.
  2. Would you mind updating setup.py to specify version 2.1.0?

@zolrath
Copy link
Contributor Author

zolrath commented Oct 19, 2017

Version bumped!

@zolrath
Copy link
Contributor Author

zolrath commented Oct 19, 2017

What's the .get() change you requested? I don't see any code comments.

NOTE: Mandatory parameters "name", "city", and "state" must be provided.
NOTE: Defaults to match_type using the "best" search method. Can be set to "lookup" for the top 10 results.
"""
if 'name' not in kwargs or not kwargs['name']:
Copy link
Collaborator

Choose a reason for hiding this comment

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

A simpler way to do this check (and all other checks like it) is the following:

if not kwargs.get('name'):

There are a few other places in this file where this pattern is found, and we should probably be using .get() for all of them.

CHANGES.md Outdated
## 2.0.1 (2017-02-27)
* Remove reliance on `ez_setup.py`.
* Add `setup.cfg` to instruct the build process to build universal wheels.

## 2.0 (2017-02-27)
* Implement the new Yelp Fusion API (aka Yelp v3 API). Note that yelpapi v1.4 is the last version to support the Yelp v2 API. [The Yelp v2 API is being deprecated, so all new developers should move to the new Fusion API ASAP.](https://engineeringblog.yelp.com/2017/02/recent-improvements-to-the-fusion-api.html)
* Use OAuth 2.0 for authentication.
* Implement new [Transaction Search API](https://www.yelp.com/developers/documentation/v3/transactions_search), [Reviews API](https://www.yelp.com/developers/documentation/v3/business_reviews), and [Autocomplete API](https://www.yelp.com/developers/documentation/v3/autocomplete).
* Use OAuth 2.0 for authentication.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd rather leave these indented since I was really just calling out specific changes related to the new Fusion API.

@gfairchild
Copy link
Collaborator

Ah, sorry! I was using GitHub's code review feature and didn't hit "submit". Do you see the comment on .get() now?

@gfairchild gfairchild changed the title Business Match API Business Match API (closes #6) Oct 19, 2017
@zolrath
Copy link
Contributor Author

zolrath commented Oct 19, 2017

I've made the requested changes!

@gfairchild
Copy link
Collaborator

Looks great to me! Merging. Thanks!

@gfairchild gfairchild merged commit 76c583a into lanl:master Oct 20, 2017
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