There is no need to pass options argument to UrlGenerator (#2273)

* There is no need to pass options argument to UrlGenerator
* Houndci compliance

lib/paperclip/attachment.rb

@@ -83,7 +83,7 @@ module Paperclip
       @errors                = {}
       @errors                = {}
       @dirty                 = false
       @dirty                 = false
       @interpolator          = options[:interpolator]
       @interpolator          = options[:interpolator]
-      @url_generator         = options[:url_generator].new(self, @options)
+      @url_generator         = options[:url_generator].new(self)
       @source_file_options   = options[:source_file_options]
       @source_file_options   = options[:source_file_options]
       @whiny                 = options[:whiny]
       @whiny                 = options[:whiny]
 
 

lib/paperclip/url_generator.rb

@@ -2,29 +2,32 @@ require 'uri'
 
 
 module Paperclip
 module Paperclip
   class UrlGenerator
   class UrlGenerator
-    def initialize(attachment, attachment_options)
+    def initialize(attachment)
       @attachment = attachment
       @attachment = attachment
-      @attachment_options = attachment_options
     end
     end
 
 
     def for(style_name, options)
     def for(style_name, options)
-      timestamp_as_needed(
-        escape_url_as_needed(
-          @attachment_options[:interpolator].interpolate(most_appropriate_url, @attachment, style_name),
-          options
-      ), options)
+      interpolated = attachment_options[:interpolator].interpolate(
+        most_appropriate_url, @attachment, style_name
+      )
+
+      escaped = escape_url_as_needed(interpolated, options)
+      timestamp_as_needed(escaped, options)
     end
     end
 
 
     private
     private
 
 
+    attr_reader :attachment
+    delegate :options, to: :attachment, prefix: true
+
     # This method is all over the place.
     # This method is all over the place.
     def default_url
     def default_url
-      if @attachment_options[:default_url].respond_to?(:call)
-        @attachment_options[:default_url].call(@attachment)
-      elsif @attachment_options[:default_url].is_a?(Symbol)
-        @attachment.instance.send(@attachment_options[:default_url])
+      if attachment_options[:default_url].respond_to?(:call)
+        attachment_options[:default_url].call(@attachment)
+      elsif attachment_options[:default_url].is_a?(Symbol)
+        @attachment.instance.send(attachment_options[:default_url])
       else
       else
-        @attachment_options[:default_url]
+        attachment_options[:default_url]
       end
       end
     end
     end
 
 
@@ -32,7 +35,7 @@ module Paperclip
       if @attachment.original_filename.nil?
       if @attachment.original_filename.nil?
         default_url
         default_url
       else
       else
-        @attachment_options[:url]
+        attachment_options[:url]
       end
       end
     end
     end
 
 

spec/paperclip/url_generator_spec.rb

@@ -4,11 +4,10 @@ require 'spec_helper'
 describe Paperclip::UrlGenerator do
 describe Paperclip::UrlGenerator do
   it "uses the given interpolator" do
   it "uses the given interpolator" do
     expected = "the expected result"
     expected = "the expected result"
-    mock_attachment = MockAttachment.new
     mock_interpolator = MockInterpolator.new(result: expected)
     mock_interpolator = MockInterpolator.new(result: expected)
+    mock_attachment = MockAttachment.new(interpolator: mock_interpolator)
 
 
-    url_generator = Paperclip::UrlGenerator.new(mock_attachment,
-                                                { interpolator: mock_interpolator })
+    url_generator = Paperclip::UrlGenerator.new(mock_attachment)
     result = url_generator.for(:style_name, {})
     result = url_generator.for(:style_name, {})
 
 
     assert_equal expected, result
     assert_equal expected, result
@@ -17,12 +16,12 @@ describe Paperclip::UrlGenerator do
   end
   end
 
 
   it "uses the default URL when no file is assigned" do
   it "uses the default URL when no file is assigned" do
-    mock_attachment = MockAttachment.new
     mock_interpolator = MockInterpolator.new
     mock_interpolator = MockInterpolator.new
     default_url = "the default url"
     default_url = "the default url"
-    options = { interpolator: mock_interpolator, default_url: default_url}
+    options = { interpolator: mock_interpolator, default_url: default_url }
+    mock_attachment = MockAttachment.new(options)
 
 
-    url_generator = Paperclip::UrlGenerator.new(mock_attachment, options)
+    url_generator = Paperclip::UrlGenerator.new(mock_attachment)
     url_generator.for(:style_name, {})
     url_generator.for(:style_name, {})
 
 
     assert mock_interpolator.has_interpolated_pattern?(default_url),
     assert mock_interpolator.has_interpolated_pattern?(default_url),
@@ -30,12 +29,12 @@ describe Paperclip::UrlGenerator do
   end
   end
 
 
   it "executes the default URL lambda when no file is assigned" do
   it "executes the default URL lambda when no file is assigned" do
-    mock_attachment = MockAttachment.new
     mock_interpolator = MockInterpolator.new
     mock_interpolator = MockInterpolator.new
     default_url = lambda {|attachment| "the #{attachment.class.name} default url" }
     default_url = lambda {|attachment| "the #{attachment.class.name} default url" }
     options = { interpolator: mock_interpolator, default_url: default_url}
     options = { interpolator: mock_interpolator, default_url: default_url}
+    mock_attachment = MockAttachment.new(options)
 
 
-    url_generator = Paperclip::UrlGenerator.new(mock_attachment, options)
+    url_generator = Paperclip::UrlGenerator.new(mock_attachment)
     url_generator.for(:style_name, {})
     url_generator.for(:style_name, {})
 
 
     assert mock_interpolator.has_interpolated_pattern?("the MockAttachment default url"),
     assert mock_interpolator.has_interpolated_pattern?("the MockAttachment default url"),
@@ -44,12 +43,16 @@ describe Paperclip::UrlGenerator do
 
 
   it "executes the method named by the symbol as the default URL when no file is assigned" do
   it "executes the method named by the symbol as the default URL when no file is assigned" do
     mock_model = FakeModel.new
     mock_model = FakeModel.new
-    mock_attachment = MockAttachment.new(model: mock_model)
-    mock_interpolator = MockInterpolator.new
     default_url = :to_s
     default_url = :to_s
-    options = { interpolator: mock_interpolator, default_url: default_url}
-
-    url_generator = Paperclip::UrlGenerator.new(mock_attachment, options)
+    mock_interpolator = MockInterpolator.new
+    options = {
+      interpolator: mock_interpolator,
+      default_url: default_url,
+      model: mock_model,
+    }
+    mock_attachment = MockAttachment.new(options)
+
+    url_generator = Paperclip::UrlGenerator.new(mock_attachment)
     url_generator.for(:style_name, {})
     url_generator.for(:style_name, {})
 
 
     assert mock_interpolator.has_interpolated_pattern?(mock_model.to_s),
     assert mock_interpolator.has_interpolated_pattern?(mock_model.to_s),
@@ -58,10 +61,10 @@ describe Paperclip::UrlGenerator do
 
 
   it "URL-escapes spaces if asked to" do
   it "URL-escapes spaces if asked to" do
     expected = "the expected result"
     expected = "the expected result"
-    mock_attachment = MockAttachment.new
     mock_interpolator = MockInterpolator.new(result: expected)
     mock_interpolator = MockInterpolator.new(result: expected)
     options = { interpolator: mock_interpolator }
     options = { interpolator: mock_interpolator }
-    url_generator = Paperclip::UrlGenerator.new(mock_attachment, options)
+    mock_attachment = MockAttachment.new(options)
+    url_generator = Paperclip::UrlGenerator.new(mock_attachment)
 
 
     result = url_generator.for(:style_name, {escape: true})
     result = url_generator.for(:style_name, {escape: true})
 
 
@@ -74,10 +77,10 @@ describe Paperclip::UrlGenerator do
         "the escaped result"
         "the escaped result"
       end
       end
     end.new
     end.new
-    mock_attachment = MockAttachment.new
     mock_interpolator = MockInterpolator.new(result: expected)
     mock_interpolator = MockInterpolator.new(result: expected)
     options = { interpolator: mock_interpolator }
     options = { interpolator: mock_interpolator }
-    url_generator = Paperclip::UrlGenerator.new(mock_attachment, options)
+    mock_attachment = MockAttachment.new(options)
+    url_generator = Paperclip::UrlGenerator.new(mock_attachment)
 
 
     result = url_generator.for(:style_name, {escape: true})
     result = url_generator.for(:style_name, {escape: true})
 
 
@@ -86,10 +89,10 @@ describe Paperclip::UrlGenerator do
 
 
   it "leaves spaces unescaped as asked to" do
   it "leaves spaces unescaped as asked to" do
     expected = "the expected result"
     expected = "the expected result"
-    mock_attachment = MockAttachment.new
     mock_interpolator = MockInterpolator.new(result: expected)
     mock_interpolator = MockInterpolator.new(result: expected)
     options = { interpolator: mock_interpolator }
     options = { interpolator: mock_interpolator }
-    url_generator = Paperclip::UrlGenerator.new(mock_attachment, options)
+    mock_attachment = MockAttachment.new(options)
+    url_generator = Paperclip::UrlGenerator.new(mock_attachment)
 
 
     result = url_generator.for(:style_name, {escape: false})
     result = url_generator.for(:style_name, {escape: false})
 
 
@@ -98,10 +101,10 @@ describe Paperclip::UrlGenerator do
 
 
   it "defaults to leaving spaces unescaped" do
   it "defaults to leaving spaces unescaped" do
     expected = "the expected result"
     expected = "the expected result"
-    mock_attachment = MockAttachment.new
     mock_interpolator = MockInterpolator.new(result: expected)
     mock_interpolator = MockInterpolator.new(result: expected)
     options = { interpolator: mock_interpolator }
     options = { interpolator: mock_interpolator }
-    url_generator = Paperclip::UrlGenerator.new(mock_attachment, options)
+    mock_attachment = MockAttachment.new(options)
+    url_generator = Paperclip::UrlGenerator.new(mock_attachment)
 
 
     result = url_generator.for(:style_name, {})
     result = url_generator.for(:style_name, {})
 
 
@@ -111,9 +114,9 @@ describe Paperclip::UrlGenerator do
   it "produces URLs without the updated_at value when the object does not respond to updated_at" do
   it "produces URLs without the updated_at value when the object does not respond to updated_at" do
     expected = "the expected result"
     expected = "the expected result"
     mock_interpolator = MockInterpolator.new(result: expected)
     mock_interpolator = MockInterpolator.new(result: expected)
-    mock_attachment = MockAttachment.new(responds_to_updated_at: false)
-    options = { interpolator: mock_interpolator }
-    url_generator = Paperclip::UrlGenerator.new(mock_attachment, options)
+    options = { interpolator: mock_interpolator, responds_to_updated_at: false }
+    mock_attachment = MockAttachment.new(options)
+    url_generator = Paperclip::UrlGenerator.new(mock_attachment)
 
 
     result = url_generator.for(:style_name, {timestamp: true})
     result = url_generator.for(:style_name, {timestamp: true})
 
 
@@ -123,9 +126,13 @@ describe Paperclip::UrlGenerator do
   it "produces URLs without the updated_at value when the updated_at value is nil" do
   it "produces URLs without the updated_at value when the updated_at value is nil" do
     expected = "the expected result"
     expected = "the expected result"
     mock_interpolator = MockInterpolator.new(result: expected)
     mock_interpolator = MockInterpolator.new(result: expected)
-    mock_attachment = MockAttachment.new(responds_to_updated_at: true, updated_at: nil)
-    options = { interpolator: mock_interpolator }
-    url_generator = Paperclip::UrlGenerator.new(mock_attachment, options)
+    options = {
+      responds_to_updated_at: true,
+      updated_at: nil,
+      interpolator: mock_interpolator,
+    }
+    mock_attachment = MockAttachment.new(options)
+    url_generator = Paperclip::UrlGenerator.new(mock_attachment)
 
 
     result = url_generator.for(:style_name, {timestamp: true})
     result = url_generator.for(:style_name, {timestamp: true})
 
 
@@ -136,9 +143,9 @@ describe Paperclip::UrlGenerator do
     expected = "the expected result"
     expected = "the expected result"
     updated_at = 1231231234
     updated_at = 1231231234
     mock_interpolator = MockInterpolator.new(result: expected)
     mock_interpolator = MockInterpolator.new(result: expected)
-    mock_attachment = MockAttachment.new(updated_at: updated_at)
-    options = { interpolator: mock_interpolator }
-    url_generator = Paperclip::UrlGenerator.new(mock_attachment, options)
+    options = { interpolator: mock_interpolator, updated_at: updated_at }
+    mock_attachment = MockAttachment.new(options)
+    url_generator = Paperclip::UrlGenerator.new(mock_attachment)
 
 
     result = url_generator.for(:style_name, {timestamp: true})
     result = url_generator.for(:style_name, {timestamp: true})
 
 
@@ -149,9 +156,9 @@ describe Paperclip::UrlGenerator do
     expected = "the?expected=result"
     expected = "the?expected=result"
     updated_at = 1231231234
     updated_at = 1231231234
     mock_interpolator = MockInterpolator.new(result: expected)
     mock_interpolator = MockInterpolator.new(result: expected)
-    mock_attachment = MockAttachment.new(updated_at: updated_at)
-    options = { interpolator: mock_interpolator }
-    url_generator = Paperclip::UrlGenerator.new(mock_attachment, options)
+    options = { interpolator: mock_interpolator, updated_at: updated_at }
+    mock_attachment = MockAttachment.new(options)
+    url_generator = Paperclip::UrlGenerator.new(mock_attachment)
 
 
     result = url_generator.for(:style_name, {timestamp: true})
     result = url_generator.for(:style_name, {timestamp: true})
 
 
@@ -162,9 +169,9 @@ describe Paperclip::UrlGenerator do
     expected = "the expected result"
     expected = "the expected result"
     updated_at = 1231231234
     updated_at = 1231231234
     mock_interpolator = MockInterpolator.new(result: expected)
     mock_interpolator = MockInterpolator.new(result: expected)
-    mock_attachment = MockAttachment.new(updated_at: updated_at)
-    options = { interpolator: mock_interpolator }
-    url_generator = Paperclip::UrlGenerator.new(mock_attachment, options)
+    options = { interpolator: mock_interpolator, updated_at: updated_at }
+    mock_attachment = MockAttachment.new(options)
+    url_generator = Paperclip::UrlGenerator.new(mock_attachment)
 
 
     result = url_generator.for(:style_name, {timestamp: false})
     result = url_generator.for(:style_name, {timestamp: false})
 
 
@@ -173,11 +180,15 @@ describe Paperclip::UrlGenerator do
 
 
   it "produces the correct URL when the instance has a file name" do
   it "produces the correct URL when the instance has a file name" do
     expected = "the expected result"
     expected = "the expected result"
-    mock_attachment = MockAttachment.new(original_filename: 'exists')
     mock_interpolator = MockInterpolator.new
     mock_interpolator = MockInterpolator.new
-    options = { interpolator: mock_interpolator, url: expected}
-
-    url_generator = Paperclip::UrlGenerator.new(mock_attachment, options)
+    options = {
+      interpolator: mock_interpolator,
+      url: expected,
+      original_filename: "exists",
+    }
+    mock_attachment = MockAttachment.new(options)
+
+    url_generator = Paperclip::UrlGenerator.new(mock_attachment)
     url_generator.for(:style_name, {})
     url_generator.for(:style_name, {})
 
 
     assert mock_interpolator.has_interpolated_pattern?(expected),
     assert mock_interpolator.has_interpolated_pattern?(expected),
@@ -186,10 +197,10 @@ describe Paperclip::UrlGenerator do
 
 
   describe "should be able to escape (, ), [, and ]." do
   describe "should be able to escape (, ), [, and ]." do
     def generate(expected, updated_at=nil)
     def generate(expected, updated_at=nil)
-      mock_attachment = MockAttachment.new(updated_at: updated_at)
       mock_interpolator = MockInterpolator.new(result: expected)
       mock_interpolator = MockInterpolator.new(result: expected)
-      options = { interpolator: mock_interpolator }
-      url_generator = Paperclip::UrlGenerator.new(mock_attachment, options)
+      options = { interpolator: mock_interpolator, updated_at: updated_at }
+      mock_attachment = MockAttachment.new(options)
+      url_generator = Paperclip::UrlGenerator.new(mock_attachment)
       def url_generator.respond_to(params)
       def url_generator.respond_to(params)
         false if params == :escape
         false if params == :escape
       end
       end

spec/support/mock_attachment.rb

 class MockAttachment
 class MockAttachment
   attr_accessor :updated_at, :original_filename
   attr_accessor :updated_at, :original_filename
+  attr_reader :options
 
 
   def initialize(options = {})
   def initialize(options = {})
+    @options = options
     @model = options[:model]
     @model = options[:model]
     @responds_to_updated_at = options[:responds_to_updated_at]
     @responds_to_updated_at = options[:responds_to_updated_at]
     @updated_at = options[:updated_at]
     @updated_at = options[:updated_at]

spec/support/mock_url_generator_builder.rb

@@ -2,9 +2,9 @@ class MockUrlGeneratorBuilder
   def initializer
   def initializer
   end
   end
 
 
-  def new(attachment, attachment_options)
+  def new(attachment)
     @attachment = attachment
     @attachment = attachment
-    @attachment_options = attachment_options
+    @attachment_options = @attachment.options
     self
     self
   end
   end