-
Notifications
You must be signed in to change notification settings - Fork 10
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
[WEBSITE-877] Refactor requests #60
[WEBSITE-877] Refactor requests #60
Conversation
katylouise
commented
Apr 7, 2017
- Created separate requests for general urls and opensearch.
- Created separate builders for opensearch and ntriples.
- Created base builder and base request.
self || super | ||
end | ||
|
||
# This class always responds to method calls, even those missing. Therefore, respond_to_missing? always returns true. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line is too long. [123/120]
@@ -0,0 +1,60 @@ | |||
module Parliament | |||
module Request | |||
class UrlRequest < Parliament::Request::BaseRequest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing top-level class documentation comment.
@query_url | ||
end | ||
|
||
def setup_query_url(search_params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Method has too many lines. [12/10]
end | ||
end | ||
|
||
def query_url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use attr_reader to define trivial reader methods.
return if url.nil? | ||
|
||
request = Parliament::Request::BaseRequest.new(base_url: url, | ||
headers: { 'Accept' => 'application/opensearchdescription+xml' }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Align the elements of a hash literal if they span more than one line.
Line is too long. [122/120]
|
||
module Parliament | ||
module Request | ||
class OpenSearchRequest < Parliament::Request::BaseRequest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing top-level class documentation comment.
# @param [Hash] params (optional) additional URI encoded form values to be added to the URI. | ||
# | ||
# @return [Parliament::Response] a Parliament::Response object containing all of the nodes returned from the URL. | ||
def get(params: nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assignment Branch Condition size for get is too high. [18.14/15]
Method has too many lines. [11/10]
# | ||
# An interesting note for #initialize is that setting base_url on the class, or using the environment variable | ||
# PARLIAMENT_BASE_URL means you don't need to pass in a base_url. You can pass one anyway to override the | ||
# environment variable or class parameter. Similarly, headers can be set by either settings the headers on the class, or passing headers in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line is too long. [147/120]
@@ -0,0 +1,136 @@ | |||
module Parliament | |||
module Request | |||
class BaseRequest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing top-level class documentation comment.
|
||
module Parliament | ||
module Builder | ||
class OpenSearchResponseBuilder < Parliament::Builder::BaseResponseBuilder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing top-level class documentation comment.
46a1a15
to
386ff3e
Compare
…se atom feed from opensearch
}.freeze | ||
|
||
def initialize(base_url: nil, headers: nil, builder: nil) | ||
@base_url = Parliament::Request::OpenSearchRequest.get_description(base_url) || self.class.base_url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we include an ENV lookup like 'OPENSEARCH_DESCRIPTION_URL'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
@@ -0,0 +1,73 @@ | |||
module Parliament | |||
module Request | |||
class OpenSearchRequest < Parliament::Request::BaseRequest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing top-level class documentation comment.
headers: { 'Accept' => 'application/opensearchdescription+xml' }) | ||
xml_response = request.get | ||
|
||
xml_root = REXML::Document.new(xml_response.body).root |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to check - is this still needed with our gem usage? Or should we use our gem too?
}.freeze | ||
|
||
def initialize(base_url: nil, headers: nil, builder: nil) | ||
@base_url = Parliament::Request::OpenSearchRequest.get_description(base_url) || self.class.base_url || ENV['OPENSEARCH_DESCRIPTION_URL'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line is too long. [144/120]