Skip to content

Use composition over inheritance for PartsList #782

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

Merged
merged 3 commits into from
Feb 26, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -41,5 +41,4 @@ matrix:
allow_failures:
- rvm: ruby-head # green, but unstable
- rvm: jruby-head # green, but unstable
- rvm: rbx-2 # see https://github.com/rubinius/rubinius/issues/3050
fast_finish: true
1 change: 1 addition & 0 deletions CHANGELOG.rdoc
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
== HEAD

* Make the gem compatible with Rubinius (Robin Dupret)
* #808 - Mail::Field correctly responds_to? the methods of its instantiated field (thegcat)
* #772 - normalize encoding matchers (grosser)
* #789 - Fix encoding collapsing not dealing with multiple encodings in 1 line (grosser)
Expand Down
33 changes: 25 additions & 8 deletions lib/mail/parts_list.rb
Original file line number Diff line number Diff line change
@@ -1,8 +1,27 @@
require 'delegate'

module Mail
class PartsList < Array
class PartsList < DelegateClass(Array)
attr_reader :parts

def initialize(*args)
@parts = Array.new(*args)
super @parts
end

# The #encode_with and #to_yaml methods are just implemented
# for the sake of backward compatibility ; the delegator does
# not correctly delegate these calls to the delegated object
def encode_with(coder) # :nodoc:
coder.represent_object(nil, @parts)
end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll want to implement respond_to_missing? as well.

Given all the boilerplate needed to implement a delegate, consider using stdlib delegate directly. May be simpler.


def to_yaml(options = {}) # :nodoc:
@parts.to_yaml(options)
end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means two PartsList objects are equal and a PartsList is equal to an array of the same parts, but a PartsList is not equal to a different PartsList object with the same parts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh I'm not sure to understand but it looks like a PartsList is equal to a different PartsList object with the same parts: http://git.io/rrPtFA.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh - not sure how that's working! Should be falling back on Object#== which is object equality.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is the case, normally here super refers to Object#== since Mail::PartsList doesn't inherit from any other object now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests?


def attachments
Mail::AttachmentsList.new(self)
Mail::AttachmentsList.new(@parts)
end

def collect
Expand All @@ -14,8 +33,6 @@ def collect
to_a
end
end

undef :map
alias_method :map, :collect

def map!
Expand All @@ -27,20 +44,20 @@ def collect!
end

def sort
self.class.new(super)
self.class.new(@parts.sort)
end

def sort!(order)
# stable sort should be used to maintain the relative order as the parts are added
i = 0;
sorted = self.sort_by do |a|
sorted = @parts.sort_by do |a|
# OK, 10000 is arbitrary... if anyone actually wants to explicitly sort 10000 parts of a
# single email message... please show me a use case and I'll put more work into this method,
# in the meantime, it works :)
[get_order_value(a, order), i += 1]
end
self.clear
sorted.each { |p| self << p }
@parts.clear
sorted.each { |p| @parts << p }
end

private
Expand Down
39 changes: 38 additions & 1 deletion spec/mail/parts_list_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,41 @@
p << html_text_part
expect(p.sort!(order)).to eq [plain_text_part, html_text_part, no_content_type_part]
end
end

it "should have a parts reader" do
p = Mail::PartsList.new([1, 2])
expect(p.parts).to eq([1, 2])
end

it "should behave like an array" do
p = Mail::PartsList.new([1, 2])
expect(p.first).to eq(1)
end

it "is equal to itself" do
p = Mail::PartsList.new([1, 2])
expect(p).to eq(p)
end

it "is equal to its parts array" do
p = Mail::PartsList.new
p << "foo"
p << "bar"
expect(p).to eq(["foo", "bar"])
end

it "can be mixed with an array" do
p = Mail::PartsList.new([3, 4])
expect([1, 2] + p).to eq [1, 2, 3, 4]
end

it "should respond to Array methods" do
p = Mail::PartsList.new
expect(p).to respond_to(:reduce)
end

it "should have a round-tripping YAML serialization" do
p = Mail::PartsList.new([1, 2])
expect(YAML.load(YAML.dump(p))).to eq(p)
end
end