Merge pull request #19 from sul-dlss/cleanup

Ticketed issues and rubocop cleanup

.gitignore

@@ -5,6 +5,7 @@
 .yardoc
 .travis
 .rvmrc
+.byebug_history
 Gemfile.lock
 InstalledFiles
 _yardoc

.rubocop_todo.yml

@@ -1,115 +1,47 @@
 # This configuration was generated by
 # `rubocop --auto-gen-config`
-# on 2016-01-06 15:42:38 -0800 using RuboCop version 0.35.1.
+# on 2016-07-19 16:41:43 -0700 using RuboCop version 0.41.2.
 # The point is for the user to remove these configuration records
 # one by one as the offenses are removed from the code base.
 # Note that changes in the inspected code, or installation of new
 # versions of RuboCop, may require this file to be generated again.
 
-# Offense count: 1
-# Cop supports --auto-correct.
-# Configuration parameters: IgnoreEmptyBlocks.
-Lint/UnusedBlockArgument:
-  Exclude:
-    - 'lib/dor-fetcher.rb'
-
-# Offense count: 1
-Metrics/AbcSize:
-  Max: 16
-
-# Offense count: 1
-Metrics/CyclomaticComplexity:
-  Max: 7
-
 # Offense count: 6
-# Configuration parameters: AllowURI, URISchemes.
+# Configuration parameters: AllowHeredoc, AllowURI, URISchemes.
+# URISchemes: http, https
 Metrics/LineLength:
   Max: 1688
 
-# Offense count: 7
-# Cop supports --auto-correct.
-# Configuration parameters: EnforcedStyle, SupportedStyles.
-Style/BracesAroundHashParameters:
-  Exclude:
-    - 'lib/dor-fetcher.rb'
-    - 'spec/dor_fetcher_spec.rb'
-
 # Offense count: 4
 Style/ClassVars:
   Exclude:
     - 'lib/dor-fetcher.rb'
 
 # Offense count: 1
-# Configuration parameters: Exclude.
 Style/Documentation:
   Exclude:
     - 'spec/**/*'
-    - 'test/**/*'
     - 'lib/dor-fetcher.rb'
 
 # Offense count: 1
-# Cop supports --auto-correct.
-# Configuration parameters: EnforcedStyle, SupportedStyles.
-Style/EmptyLinesAroundBlockBody:
-  Exclude:
-    - 'spec/dor_fetcher_spec.rb'
-
-# Offense count: 1
-# Configuration parameters: Exclude.
+# Configuration parameters: ExpectMatchingDefinition, Regex, IgnoreExecutableScripts.
 Style/FileName:
   Exclude:
     - 'lib/dor-fetcher.rb'
 
-# Offense count: 32
+# Offense count: 28
 # Cop supports --auto-correct.
-# Configuration parameters: EnforcedStyle, SupportedStyles, UseHashRocketsWithSymbolValues.
+# Configuration parameters: SupportedStyles, UseHashRocketsWithSymbolValues, PreferHashRocketsForNonAlnumEndingSymbols.
+# SupportedStyles: ruby19, ruby19_no_mixed_keys, hash_rockets
 Style/HashSyntax:
-  Enabled: false
-
-# Offense count: 1
-# Cop supports --auto-correct.
-# Configuration parameters: EnforcedStyle, SupportedStyles.
-Style/MultilineOperationIndentation:
-  Enabled: false
-
-# Offense count: 2
-# Cop supports --auto-correct.
-Style/NumericLiterals:
-  MinDigits: 9
+  EnforcedStyle: hash_rockets
 
 # Offense count: 1
 # Configuration parameters: NamePrefix, NamePrefixBlacklist, NameWhitelist.
+# NamePrefix: is_, has_, have_
+# NamePrefixBlacklist: is_, has_, have_
+# NameWhitelist: is_a?
 Style/PredicateName:
   Exclude:
+    - 'spec/**/*'
     - 'lib/dor-fetcher.rb'
-
-# Offense count: 1
-# Cop supports --auto-correct.
-# Configuration parameters: EnforcedStyle, SupportedStyles.
-Style/SignalException:
-  Exclude:
-    - 'lib/dor-fetcher.rb'
-
-# Offense count: 3
-# Cop supports --auto-correct.
-# Configuration parameters: SupportedStyles, EnforcedStyleForEmptyBraces, SpaceBeforeBlockParameters.
-Style/SpaceInsideBlockBraces:
-  EnforcedStyle: no_space
-
-# Offense count: 2
-# Cop supports --auto-correct.
-Style/SpaceInsideBrackets:
-  Exclude:
-    - 'Rakefile'
-
-# Offense count: 22
-# Cop supports --auto-correct.
-# Configuration parameters: EnforcedStyle, EnforcedStyleForEmptyBraces, SupportedStyles.
-Style/SpaceInsideHashLiteralBraces:
-  Enabled: false
-
-# Offense count: 1
-# Cop supports --auto-correct.
-# Configuration parameters: WordRegex.
-Style/WordArray:
-  MinSize: 6

Rakefile

@@ -33,7 +33,7 @@ begin
 
   YARD::Rake::YardocTask.new(:doc) do |yt|
     yt.files = Dir.glob(File.join(project_root, 'lib', '**', '*.rb')) +
-                 [ File.join(project_root, 'README.rdoc') ]
+               [File.join(project_root, 'README.rdoc')]
     yt.options = ['--output-dir', doc_dest_dir, '--readme', 'README.rdoc', '--title', 'Harvestdor Gem Documentation']
   end
 rescue LoadError

dor-fetcher.gemspec

@@ -1,14 +1,16 @@
 Gem::Specification.new do |s|
   s.name        = 'dor-fetcher'
-  s.version     = '1.1.7'
+  s.version     = '1.2.0'
   s.date        = '2014-12-18'
   s.summary     = 'DorFetcher Gem'
   s.description = 'Wrapper for the Dor Fetcher Services RESTful API.'
   s.authors     = ['Carrick Rogers', 'Laney McGlohon', 'Bess Sadler']
   s.email       = ['carrickr@stanford.edu', 'laneymcg@stanford.edu', 'bess@stanford.edu']
   s.files       = Dir.glob('lib/**/*')
-  s.homepage    = 'http://www.stanford.edu'
+  s.homepage    = 'https://github.com/sul-dlss/dor-fetcher'
   s.license     = 'Apache-2.0'
+  s.add_development_dependency 'rake'
+  s.add_development_dependency 'rubocop'
   s.add_development_dependency 'rspec'
   s.add_development_dependency 'vcr'
   s.add_development_dependency 'webmock'

lib/dor-fetcher.rb

@@ -12,16 +12,16 @@ class Client
     attr_reader :service_url # Base URL this instance will run RESTful API calls against
 
     # Create a new instance of DorFetcher::Client
-    # @param options [Hash] Currently supports :service_url and :skip_heartbeat.
-    # @param :service_url is the base url for API queries.  Defaults to http://127.0.0.1:3000
-    # @param :skip_heartbeat will tell the init to skip querying the :service_url and seeing if the API is responsive
+    # @param options [Hash]
+    # @option options [String] :service_url base url for API queries.  Default: http://127.0.0.1:3000
+    # @option options [Boolean] :skip_heartbeat skip querying :service_url to confirm API is responsive.  Default: false
     # @example
-    #    df = DorFetcher::Client.new({:service_url='http://SERVICEURL'})
+    #    df = DorFetcher::Client.new(:service_url => 'http://SERVICEURL')
     def initialize(options = {})
       # TODO: Check for a well formed URL and a 200 from the destination before just accepting this
       @service_url = options[:service_url] || @@default_service_url
       @site = RestClient::Resource.new(@service_url)
-      raise "DorFetcher::Client Error! No response from #{@service_url}" unless options[:skip_heartbeat] || self.is_alive?
+      raise "DorFetcher::Client Error! No response from #{@service_url}" unless options[:skip_heartbeat] || is_alive?
     end
 
     # Return service info (rails env, version deployed, last restart and last deploy)
@@ -42,45 +42,41 @@ def is_alive?
     #
     # @param collection [String] we expect pid/druid
     # @param params [Hash] we expect :count_only or any of @@supported_params
-    # @return [Hash] Hash of all objects in the collection including
-    # pid/druid, title, date last modified, and count
+    # @return [Hash] Hash of all objects in the collection including: pid/druid, title, date last modified, and count
     def get_collection(collection, params = {})
       query_api('collections', collection, params)
     end
 
     # Get the count of the number of items in a collection, including the collection object itself
     # @param collection [String] we expect pid/druid
-    # @param params [Hash] we expect :count_only or any of @@supported_params
+    # @param params [Hash] any of @@supported_params
     # @return [Integer] Number found
     def get_count_for_collection(collection, params = {})
-      query_api('collections', collection, add_count_only_param(params))
+      query_api('collections', collection, params.merge!(:count_only => true))
     end
 
     # Get a Hash of all the collections in the digital repository that are accessioned
-    # @return [Hash] Hash of all collections including pid/druid, title,
-    # date last modified, and count
+    # @return [Hash] All collections including: pid/druid, title, date last modified, and count
     def list_all_collections
       query_api('collections', '', {})
     end
 
     # Get a Hash of all the collections in the digital repository
-    # @return [Hash] Hash of all collections including pid/druid, title,
-    # date last modified, and count
+    # @return [Hash] All registered collections including: pid/druid, title, date last modified, and count
     def list_registered_collections
-      query_api('collections', '', {:status => 'registered'})
+      query_api('collections', '', :status => 'registered')
     end
 
     # Get a Count of all the collections in the digital repository
     # @return [Integer] Number of all collections
     def total_collection_count
-      query_api('collections', '', {:count_only => true})
+      query_api('collections', '', :count_only => true)
     end
 
     # Get the APO and all objects governed by the APO
     # @param apo [String] pid/druid of the APO
     # @param params [Hash] we expect :count_only or any of @@supported_params
-    # @return [Hash] Hash of all objects governed by the APO including
-    # pid/druid, title, date last modified, and count
+    # @return [Hash] All objects governed by the APO including: pid/druid, title, date last modified, and count
     def get_apo(apo, params = {})
       query_api('apos', apo, params)
     end
@@ -91,34 +87,32 @@ def get_apo(apo, params = {})
     # @param params [Hash] we expect :count_only or any of @@supported_params
     # @return [Integer] Number found
     def get_count_for_apo(apo, params = {})
-      query_api('apos', apo, add_count_only_param(params))
+      query_api('apos', apo, params.merge!(:count_only => true))
     end
 
     # Get a Hash of all the APOs in the digital repository that are accessioned
-    # @return [Hash] Hash of all APOs including pid/druid, title,
-    # date last modified, and count
+    # @return [Hash] All APOs including: pid/druid, title, date last modified, and count
     def list_all_apos
       query_api('apos', '', {})
     end
 
     # Get a Hash of all the APOs in the digital repository that are registered
-    # @return [Hash] Hash of all APOs including pid/druid, title,
-    # date last modified, and count
+    # @return [Hash] All registered APOs including: pid/druid, title, date last modified, and count
     def list_registered_apos
-      query_api('apos', '', {:status => 'registereed'})
+      query_api('apos', '', :status => 'registered')
     end
 
     # Get a Count of all the APOs in the digital repository
     # @return [Integer] Number of all APOs
     def total_apo_count
-      query_api('apos', '', {:count_only => true})
+      query_api('apos', '', :count_only => true)
     end
 
-    # Method to parse full Hash into an array containing only the druids
-    #
+    # Parses full Hash into an array containing only the druids
     # @param response [Hash] Hash as returned by query_api
-    # @param no_prefix [boolean] if true (default), remove the druid: prefix on all druids, if false, leave alone
-    # @return [Array] the array listing all druids in the supplied Hash
+    # @param params [Hash{Symbol=>Boolean}] options
+    # @option params [Boolean] :no_prefix if true (default), remove the 'druid:' prefix on all druids
+    # @return [Array{String}] all druids in the supplied Hash
     def druid_array(response, params = {})
       return_list = []
       response.each do |key, items|
@@ -132,20 +126,30 @@ def druid_array(response, params = {})
       return_list
     end
 
+    # Synthesize URL from base, druid and params
+    # @see #query_api for args
+    # @return [String] URL
+    def query_url(base, druid, params)
+      url = "#{@site}/#{base}"
+      url += "/#{druid}" unless druid.nil? || druid.empty?
+      url += add_params(params).to_s unless params.nil? || params.empty?
+      url
+    end
+
     # Query a RESTful API and return the JSON result as a Hash
     # @param base [String] The name of controller of the Rails App you wish to route to
-    # @param druid [String] The druid/pid of the object you wish to query,or empty string for no specific druid
+    # @param druid [String] The druid/pid of the object you wish to query, or empty string for no specific druid
     # @param params [Hash] we expect :count_only or any of @@supported_params
-    # @return [Hash] Hash of all objects governed by the APO including pid/druid, title, date last modified, and count
+    # @option params [Hash] :count_only
+    # @return [Hash,Integer] All objects governed by the APO including pid/druid, title, date last modified, and count -- or just the count if :count_only
     def query_api(base, druid, params)
-      url = "#{@site}/#{base}"
-      url += "/#{druid}" unless druid.nil? || druid.empty?
-      url += "#{add_params(params)}" unless params.nil? || params.empty?
+      url = query_url(base, druid, params)
       begin
-        # We need to use this method here for the longer timeout option
-        resp = RestClient::Request.execute(:method => :get, :url => url, :timeout => 90000000, :open_timeout => 90000000)
-      rescue
-        raise "Connection Error with url #{url}"
+        # We use RestClient::Request.execute here for the longer timeout option
+        resp = RestClient::Request.execute(:method => :get, :url => url, :timeout => 180)
+      rescue RestClient::Exception => e
+        warn "Connection Error with url #{url}: #{e.message}"
+        raise e
       end
 
       # RestClient monkey patches its response so it looks like a string, but really isn't.
@@ -155,24 +159,12 @@ def query_api(base, druid, params)
     end
 
     # Transform a parameter hash into a RESTful API parameter format
-    #
-    # @param input_params [Hash] {The existing parameters, eg time and tag}
+    # @param input_params [Hash{Symbol=>Object}] The existing parameters, eg time and tag
     # @return [String] parameters in the Hash now formatted into a RESTful parameter string
     def add_params(input_params)
-      input_params.delete_if { |key, value| !@@supported_params.include?(key)}
       uri = Addressable::URI.new
-      uri.query_values = input_params
-      qs = uri.query.gsub('count_only=true', @@count_only_param)
-      "?#{qs}"
-    end
-
-    # Add the parameter so query_api knows only to get a count of the documents in solr
-    #
-    # @param params [Hash] {The existing parameters, eg time and tag}
-    # @return [Hash] the params Hash plus the key/value set :count_only=>true
-    def add_count_only_param(params)
-      params.store(:count_only, true)
-      params
+      uri.query_values = input_params.select { |key, _val| @@supported_params.include?(key) }
+      '?' + uri.query.gsub('count_only=true', @@count_only_param)
     end
   end
 end