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

Add search_engine parameter to Users endpoint + tests #115

Merged
merged 2 commits into from
Jul 5, 2018

Conversation

joshcanhelp
Copy link
Contributor

  • Replace the hard-coded search engine version v2 with a modifiable one
  • Improve docs for the method
  • Add unit and integration tests
  • Remove skipped user test

@joshcanhelp joshcanhelp added this to the v4-Next milestone Jun 28, 2018
@joshcanhelp joshcanhelp requested a review from machuga June 28, 2018 22:40
Copy link

@machuga machuga left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -27,9 +30,9 @@ def users(options = {})
connection: options.fetch(:connection, nil),
fields: options.fetch(:fields, nil),
include_fields: options.fetch(:include_fields, nil),
q: options.fetch(:q, nil)
q: options.fetch(:q, nil),
Copy link

Choose a reason for hiding this comment

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

Nothing you need to worry about now since the convention is used, but just using as an opportunity to share info:

.fetch is typically used on a hash when you want to look for a key and raise an exception if it is not empty, or return a specific default. Accessing a hash key via the [] method is equivalent to .fetch(key, :nil), so when nil is the desired default return value, you'll typically find the [] method is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@machuga - So something like:

request_params = {
  ...
  q: [:q]
  ...
}

?

Copy link

Choose a reason for hiding this comment

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

Close:

options[:key] === options.fetch(:key, nil)

@joshcanhelp joshcanhelp merged commit bd36202 into master Jul 5, 2018
@joshcanhelp joshcanhelp deleted the add-search-engine-to-users branch July 5, 2018 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants