Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

SearchResults object contains metadata and results array #261

Merged
merged 7 commits into from Jun 1, 2012

Conversation

Projects
None yet
3 participants
Contributor

wjlroe commented May 24, 2012

Refactor the Twitter::Client#search function to return a SearchResults object that not only contains the results (.results) but also metadata about the search such the the .max_id (useful for searching again using the :since_id parameter.

@sferik sferik commented on the diff May 24, 2012

lib/twitter/search_results.rb
+module Twitter
+ class SearchResults < Twitter::Base
+
+ lazy_attr_reader :results, :completed_in, :max_id, :max_id_str, :next_path, :page,
+ :query, :refresh_url, :results_per_page, :since_id, :since_id_str
+
+
+ # @return [Array<Twitter::Status>]
+ def results
+ @results ||= (@attrs['results'] || []).map{ |status| Twitter::Status.new(status) }
+ end
+
+ # @return [Float]
+ def completed_in
+ @attrs['completed_in']
+ end
@sferik

sferik May 24, 2012

Owner

It is not necessary to define basic reader methods if you've declared a lazy_attr_reader. This is precisely what lazy_attr_reader provides.

@sferik sferik commented on the diff May 24, 2012

lib/twitter/search_results.rb
@@ -0,0 +1,66 @@
+require 'twitter/base'
+
+module Twitter
+ class SearchResults < Twitter::Base
+
+ lazy_attr_reader :results, :completed_in, :max_id, :max_id_str, :next_path, :page,
+ :query, :refresh_url, :results_per_page, :since_id, :since_id_str
+
+
+ # @return [Array<Twitter::Status>]
+ def results
+ @results ||= (@attrs['results'] || []).map{ |status| Twitter::Status.new(status) }
+ end
@sferik

sferik May 24, 2012

Owner

Likewise, it's not necessary to declare a lazy_attr_reader if you're going to define a custom reader, like this one. It should be one or the other.

@sferik

sferik May 24, 2012

Owner

Consider aliasing collection to results to mirror the Twitter::Cursor class, which server a similar function as an envelope/wrapper class.

@sferik sferik commented on the diff May 24, 2012

lib/twitter/search_results.rb
@@ -0,0 +1,66 @@
+require 'twitter/base'
+
+module Twitter
+ class SearchResults < Twitter::Base
@sferik

sferik May 24, 2012

Owner

Please add specs for this new class.

@sferik sferik commented on an outdated diff May 24, 2012

lib/twitter/client.rb
@@ -1787,17 +1789,12 @@ def saved_search_destroy(id, options={})
# @option options [Integer] :since_id Returns results with an ID greater than (that is, more recent than) the specified ID. There are limits to the number of Tweets which can be accessed through the API. If the limit of Tweets has occured since the since_id, the since_id will be forced to the oldest ID available.
# @option options [Integer] :max_id Returns results with an ID less than (that is, older than) or equal to the specified ID.
# @option options [Boolean, String, Integer] :with_twitter_user_id When set to either true, t or 1, the from_user_id and to_user_id values in the response will map to "official" user IDs which will match those returned by the REST API.
- # @return [Array<Twitter::Status>] Return tweets that match a specified query
+ # @return <Twitter::SearchResults> Return tweets that match a specified query with search metadata
@sferik

sferik May 24, 2012

Owner

Twitter::SearchResults should be encased in square brackets ([]) not angle brackets (<>).

@sferik sferik and 2 others commented on an outdated diff May 24, 2012

lib/twitter/client.rb
@@ -1,3 +1,4 @@
+# -*- coding: utf-8 -*-
@sferik

sferik May 24, 2012

Owner

Why was it necessary to add this?

@richo

richo May 25, 2012

Is there a defined convention for this?

I'm never sure whether to add modelines to other people's project, the heuristic I've been running with is if their project deviates from the standard (3 space indents or something) I add them.

@sferik

sferik May 25, 2012

Owner

You should only need to add this line of the source file contains non-ASCII characters. If you're not adding such characters, there's no need to add this like.

@wjlroe

wjlroe May 25, 2012

Contributor

There are some non-ascii hyphens in the docs of this file which was why Emacs was automatically adding this encoding hint. I have committed a version with ascii hyphens instead

@sferik sferik commented on an outdated diff May 24, 2012

spec/twitter/client/search_spec.rb
@@ -20,9 +20,14 @@
end
it "should return recent statuses related to a query with images and videos embedded" do
search = @client.search('twitter')
- search.should be_an Array
- search.first.should be_a Twitter::Status
- search.first.text.should == "@KaiserKuo from not too far away your new twitter icon looks like Vader."
+ search.results.should be_an Array
+ search.results.first.should be_a Twitter::Status
+ search.results.first.text.should == "@KaiserKuo from not too far away your new twitter icon looks like Vader."
@sferik

sferik May 24, 2012

Owner

You probably want to assert that the response is, in fact, a Twitter:: SearchResults object, no?

Owner

sferik commented May 24, 2012

Overall, this change looks fine but there are a few small issues that need to be fixed before we can pull.

@sferik sferik added a commit that referenced this pull request Jun 1, 2012

@sferik sferik Merge pull request #261 from wjlroe/search-metadata
SearchResults object contains metadata and results array
402792c

@sferik sferik merged commit 402792c into sferik:master Jun 1, 2012

@sferik sferik added a commit that referenced this pull request Jun 1, 2012

@sferik sferik Clean up #261 7a96bcd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment