replace Paperclip::Attachment#initialize options with an AttachmentOptions instance #729

Merged
merged 5 commits into from Feb 3, 2012

Projects

None yet

3 participants

@Sporky023
Contributor

No description provided.

@sikachu
Contributor
sikachu commented Feb 3, 2012

Go ahead and merge this in, Luke.

@Sporky023
Contributor

Sorry in advance about this:

There is now completely new work in this pull request that's not related to the original ctags change.

The new work is relating to how options are passed in to Paperclip::Attachment.new - using an object which inherits from Hash.

This warrants a completely new code review.

Also, sikachu, I don't believe I have access to push to github.com:thoughtbot/paperclip.git. That's why I've been sending pull requests from my own fork.

@mike-burns mike-burns commented on an outdated diff Feb 3, 2012
test/attachment_options_test.rb
@@ -0,0 +1,45 @@
+require './test/helper'
+
+class AttachmentOptionsTest < Test::Unit::TestCase
+ should "exist" do
+ Paperclip::AttachmentOptions
+ end
@mike-burns
mike-burns Feb 3, 2012 Member

This test is implied by other, more behavioral tests.

@mike-burns mike-burns commented on an outdated diff Feb 3, 2012
test/attachment_options_test.rb
@@ -0,0 +1,45 @@
+require './test/helper'
+
+class AttachmentOptionsTest < Test::Unit::TestCase
+ should "exist" do
+ Paperclip::AttachmentOptions
+ end
+
+ should "be a Hash" do
+ attachment_options = Paperclip::AttachmentOptions.new({})
+ assert attachment_options.is_a?(Hash), "attachment_options is not a Hash"
@mike-burns
mike-burns Feb 3, 2012 Member
assert_kind_of
@mike-burns mike-burns commented on an outdated diff Feb 3, 2012
test/attachment_options_test.rb
+
+ should "add a default empty validations" do
+ options = {:arbi => :trary}
+ expected = {:validations => []}.merge(options)
+ actual = Paperclip::AttachmentOptions.new(options).to_hash
+ assert_equal expected, actual
+ end
+
+ should "not override validations if passed to initializer" do
+ options = {:validations => "something"}
+ attachment_options = Paperclip::AttachmentOptions.new(options)
+ assert_equal "something", attachment_options[:validations]
+ end
+
+ should "respond to []" do
+ Paperclip::AttachmentOptions.new({})[:foo]
@mike-burns
mike-burns Feb 3, 2012 Member
assert Paperclip::AttachmentOptions.new({}).respond_to(:[])

Bonus, you get to use (:[]), which looks like a monkey.

@mike-burns mike-burns and 1 other commented on an outdated diff Feb 3, 2012
test/paperclip_test.rb
@@ -98,6 +98,13 @@ class Dummy2 < ActiveRecord::Base
end
end
+ context "An ActiveRecord model responding to has_attached_file" do
@mike-burns
mike-burns Feb 3, 2012 Member

This is testing deep inside the implementation, which will make it harder to refactor. Is there some behavior you need to test, or perhaps it's already testing and this is just a refactoring.

@Sporky023
Sporky023 Feb 3, 2012 Contributor

I think I'll just remove this test

@mike-burns mike-burns commented on an outdated diff Feb 3, 2012
test/mocks/mock_attachment_options.rb
@@ -0,0 +1,5 @@
+class MockAttachmentOptions
@mike-burns
mike-burns Feb 3, 2012 Member

This is unused.

@Sporky023 Sporky023 merged commit 7f948f8 into thoughtbot:master Feb 3, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment