Skip to content

#11 - Updating url decorator to allow for place.scope/exact #12

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jul 23, 2017
Merged

#11 - Updating url decorator to allow for place.scope/exact #12

merged 3 commits into from
Jul 23, 2017

Conversation

robertkern
Copy link
Member

No description provided.

@robertkern robertkern requested a review from braedon July 18, 2017 19:23
@coveralls
Copy link

coveralls commented Jul 18, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 100.0% when pulling b760a6b on 11-place into 496f503 on master.

@coveralls
Copy link

coveralls commented Jul 18, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 100.0% when pulling a8cee49 on 11-place into 496f503 on master.

braedon
braedon reviewed Jul 18, 2017
View reviewed changes
tests/endpoints/v1/events_tests.py Outdated
client.events.search(id="id", q="query", rank_level=[4,5], rank__gt=85, country=["NZ", "AU"],
within__radius="2km", within__longitude=-71.0432, within__latitude=42.346,
label=["label1", "label2"], category="category",
place__scope=["place1", "place2"], place__exact=["place3"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we not supporting the __ format? If not, should change all the other param tests as well. If we are, should have tests in both forms for all params.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah good point, it's already supported so will update the tests

braedon
braedon approved these changes Jul 18, 2017
View reviewed changes
Copy link
Contributor

@braedon braedon left a comment

Choose a reason for hiding this comment

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

LGTM

@coveralls
Copy link

coveralls commented Jul 18, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 100.0% when pulling 2e7afe5 on 11-place into 496f503 on master.

hampsterx
hampsterx approved these changes Jul 23, 2017
View reviewed changes
Copy link
Contributor

@hampsterx hampsterx left a comment

Choose a reason for hiding this comment

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

nice catch, seems good. The reason for "country" not "place" example on readme was to reduce confusion for first time viewer.

@robertkern robertkern merged commit 69b96cb into master Jul 23, 2017
@robertkern robertkern deleted the 11-place branch July 23, 2017 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@braedon braedon braedon approved these changes

@hampsterx hampsterx hampsterx approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

in progress

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@robertkern @coveralls @braedon @hampsterx