Use composition over inheritance for `PartsList` #782

robin850 · 2014-08-17T14:17:11Z

Hello,

This pull request makes this gem compatible with Rubinius relying on the composition over inheritance technique. Even Rails no longer uses the inheritance pattern (see this article and rails/rails#16299).

For now, there is a compatibility layer that delegates any missing methods to the parts list's @parts so it behaves like an array. This is up to you but you may need to deprecate this path and tell people to access #parts first, then call the given method or provide your very own API (through Enumerable for instance).

If you want me to squash some commits together let me know !

Refs rubinius/rubinius#3050.

This has been tested under MRI 2.1.2 and 1.9.3, JRuby 1.7.13 and Rubinius 2.2.10.

Have a nice day.

robin850 · 2014-08-23T15:05:13Z

The commit that removes Rubinius from the allowed failures matrix is here because the whole purpose of this pull request is to make mail compatible with Rubinius.
Regarding the #== thing, this has been fixed ; good catch !
As for aliasing #to_ary and #parts, anyway this is not the right fix. We should make the different implementations behave as close as possible. Defining #respond_to? seems more adequate as the behavior was wrong previously on MRI and now Array#+ works fine on JRuby.
Since public_send doesn't exist in 1.8.7, I took the liberty to add a #respond_to? call inside #method_missing. Being able to call private methods is not ideal.
Regarding the round-trip problem with YAML (de)serialization, I took the liberty to remove the method. Actually this wasn't round-tripping before ; serializing a Mail::PartsList and loading it would return an array. This seems more logical and is mentioned in the CHANGELOG. What do you think ?

Thanks for your feedback @jeremy ! :-)

jeremy · 2014-08-24T22:17:17Z

lib/mail/parts_list.rb

+
+    def ==(other)
+      super || @parts == other
+    end


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.

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.

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

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

robin850 · 2014-08-27T17:34:21Z

Thanks for your review @jeremy!

Regarding YAML seralization, finally, we are serializing the @parts array instead of making YAML round-trips for the sake of backward-compatibility ; the upgrade will be smoother as we may already introduce some regressions with this new "layer". The behavior is weird but it's safer to keep it as is. What do you think ?

The build is still failing due to a problem with RSpec apparently ; this was already the case on master. 😢 Action Mailer is all green on Rubinius (2.2.10), JRuby (1.7.13) and MRI (2.1.2) with this branch though.

ryan2johnson9 · 2014-10-20T06:25:13Z

Hi, I'm a newbie to github- I'm suffering from the problem this pull-request solves. Is it possible for me to get this change or do I need to wait for someone to merge the pull request?

robin850 · 2014-10-20T16:02:54Z

@ryan2johnson9 : You'll need to wait until it's merged and it gets included inside a release or reference this branch directly inside your Gemfile:

gem 'mail', github: 'robin850/mail', branch: 'composition-parts-list'

or apply the patch on a local copy of the gem; There is still some work to do though (especially regarding @bf4's comment) ; I will try to do it as soon as possible so it can be merged. Thanks for the reminder!

ryan2johnson9 · 2014-10-20T19:40:36Z

Ahh, thanks so much!!

bf4 · 2014-11-03T21:38:43Z

It appears the rspec error messages contain invalid utf-8 so rubinius is blowing up

monkeypatch I've submitted as a PR to rspec-core rspec/rspec-core#1760

require 'rspec/notifications'
class RSpec::Core::Notifications::FailedExampleNotification
  private
     def exception_message
      @exception_message ||= exception.message.to_s.
        # Converting it to a higher higher character set (UTF-16) and then
        # back (to UTF-8) ensures that you will strip away invalid or undefined byte sequences.
        encode(Encoding::UTF_16LE, :invalid => :replace, :undef => :replace, :replace => '?').
        encode(Encoding::UTF_8)
    end
    def failure_lines
      @failure_lines ||=
        begin
          lines = ["Failure/Error: #{read_failed_line.strip}"]
          lines << "#{exception_class_name}:" unless exception_class_name =~ /RSpec/
          exception_message.split("\n").each do |line|
            lines << "  #{line}" if exception.message
          end
          lines
        end
    end
end

bf4 · 2014-11-03T22:58:54Z

I've confirmed the tests pass on rbx, when I apply

commit a1f454d8a26ec822352fcb7eae9db02629f1da4c
Author: Benjamin Fleischer <[email protected]>
Date:   Mon Nov 3 16:52:26 2014 -0600

    use utf8-friendly rspec-core

diff --git a/Gemfile b/Gemfile
index 8228d32..03df3f1 100644
--- a/Gemfile
+++ b/Gemfile
@@ -1,5 +1,12 @@
 source "https://rubygems.org"

+%w{rspec-support rspec-expectations rspec-mocks}.each do |gem_name|
+  gem gem_name, github: "rspec/#{gem_name}"
+end
+gem 'rspec-core', github: 'bf4/rspec-core', branch: 'handle_non_utf8_exception_messages'
+gem 'rspec', github: 'rspec/rspec'
 gemspec

 gem "tlsmail", "~> 0.0.1" if RUBY_VERSION <= "1.8.6"
diff --git a/mail.gemspec b/mail.gemspec
index 7375c30..28f9cc7 100644
--- a/mail.gemspec
+++ b/mail.gemspec
@@ -18,7 +18,7 @@ Gem::Specification.new do |s|

   s.add_development_dependency('bundler', '>= 1.0.3')
   s.add_development_dependency('rake', '> 0.8.7')
-  s.add_development_dependency('rspec', '~> 3.0.0')
+  s.add_development_dependency('rspec', '~> 3.0')
   s.add_development_dependency('rdoc')

   s.files = %w(README.md MIT-LICENSE CONTRIBUTING.md CHANGELOG.rdoc Dependencies.txt Gemfile Rakefile TODO.rdoc) + Dir.glob("lib/**/*")

bf4 · 2014-11-04T01:40:37Z

lib/mail/parts_list.rb

+
+    def to_yaml(options = {})
+      @parts.to_yaml(options)
+    end


robin850 · 2015-01-01T18:25:35Z

Ok, sorry it took so long to get back to you but the feedback seems to be addressed. :-) @bf4 : Thanks a ton for working on making RSpec more compliant with Rubinius ; hoping the pull request will be grabbed soon ! ❤️

Do you want me to put back Rubinius in the list of allowed failures ? At least the JRuby build is broken as well due to travis-ci/travis-ci#3067.

it seems that represent_object isn't available for < 1.9.3

Is it really a problem ? In Ruby 1.9.2 and less, the default YAML engine is Syck so the encode_with method won't ever be called.

Let me know if there's anything missing! Happy new year to you guys! 🎉

bf4 · 2015-02-09T16:26:01Z

Trying this with RSpec head https://travis-ci.org/mikel/mail/builds/50076945

bjfish · 2015-02-21T19:56:59Z

@robin850 This PR built successfully for me locally with rubinius, maybe reopen the PR to trigger a new build?

robin850 · 2015-02-24T11:15:02Z

It looks like you're right @bjfish, everything is green now ! \o/

@bf4 : Is there anything missing before shipping it please ? :-)

bf4 · 2015-02-24T20:14:28Z

Looks good to me @jeremy @mikel

Instead of inheriting from `Array`, let's create an attribute that is an array. The current object doesn't fit with the Liskov substitution principle as, for instance, #sort is broken on Rubinius. This makes the `PartsList` class compatible with Rubinius as `undef map` will remove the map method but some other Enumerable methods (e.g. sort) rely on it. Also add fallback for Array's methods for the sake of backward compatibility, we need to provide a fallback for missing method that may be used in the existing libraries. To avoid all the necessary boilerplate to provide such fallback, let's rely on the standard library's delegate gem and its DelegateClass method.

Previously, the object would have been serialized as an Array even though YAML wouldn't be able to round-trip. Let's keep this behavior for now to make upgrade smoother to this new "layer".

Use composition over inheritance for `PartsList`

The edge version ships with a patch that uses composition over inheritance for the Mail::PartsList object (see mikel/mail#782). Let's test Action Mailer against it to prevent eventual regressions and experience it. Moreover, this branch makes the Action Mailer suite green against Rubinius.

Use composition over inheritance for `PartsList`

There's a bug in the mail gem that only affects rubinius. This seems to be causing build failures for mailgun_rails. Here's the details of the bug: mikel/mail#782 This commit temporarily sets travis to ignore failures on rubinius. I think it's better to have a reliable build than have to manually check whether the failure is due to a known bug in another library. Ultimately this should be reverted once the bug is resolved.

jeremy reviewed Aug 24, 2014
View reviewed changes

jc00ke mentioned this pull request Sep 5, 2014

Possible bug in behavior of sort! rubinius/rubinius#3050

Closed

bf4 mentioned this pull request Sep 30, 2014

method 'sort!': given 0, expected 1 #803

Closed

bf4 reviewed Nov 4, 2014
View reviewed changes

lib/mail/parts_list.rb

def to_yaml(options = {})

@parts.to_yaml(options)

end

Copy link

Contributor

bf4 Nov 4, 2014

Choose a reason for hiding this comment

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

tests?

robin850 added 3 commits February 26, 2015 21:40

Let's remove Rubinius from the allowed failures

3bf3671

Make YAML serialization backward-compatible

30ed6c5

Previously, the object would have been serialized as an Array even though YAML wouldn't be able to round-trip. Let's keep this behavior for now to make upgrade smoother to this new "layer".

jeremy added a commit that referenced this pull request Feb 26, 2015

Merge pull request #782 from robin850/composition-parts-list

b159e0a

Use composition over inheritance for `PartsList`

jeremy merged commit b159e0a into mikel:master Feb 26, 2015

robin850 deleted the composition-parts-list branch February 26, 2015 20:51

robin850 restored the composition-parts-list branch February 26, 2015 20:52

robin850 mentioned this pull request Mar 2, 2015

Improve the Rubinius build rails/rails#19173

Merged

TylerRick pushed a commit to TylerRick/mail that referenced this pull request Mar 4, 2015

Merge pull request mikel#782 from robin850/composition-parts-list

676d53d

Use composition over inheritance for `PartsList`

This was referenced Mar 16, 2015

Build times out on RBX mailman/mailman#116

Open

Temporary fix for #116 mailman/mailman#117

Merged

iainbeeston mentioned this pull request Apr 1, 2015

Build fails on rubinius jorgemanrubia/mailgun_rails#43

Closed

iainbeeston mentioned this pull request Apr 1, 2015

Ignored rubinius build failures on travis jorgemanrubia/mailgun_rails#44

Merged

This was referenced Nov 2, 2017

Fix "wrong number of arguments (given 1, expected 0)" in "sort!" #1148

Closed

Message#from_yaml results in invalid parts when body is not present #1038

Closed

Use composition over inheritance for PartsList #782

Use composition over inheritance for PartsList #782

Uh oh!

Conversation

robin850 commented Aug 17, 2014

Uh oh!

robin850 commented Aug 23, 2014

Uh oh!

jeremy Aug 24, 2014

Choose a reason for hiding this comment

Uh oh!

robin850 Aug 25, 2014

Choose a reason for hiding this comment

Uh oh!

jeremy Aug 25, 2014

Choose a reason for hiding this comment

Uh oh!

robin850 Aug 25, 2014

Choose a reason for hiding this comment

Uh oh!

robin850 commented Aug 27, 2014

Uh oh!

ryan2johnson9 commented Oct 20, 2014

Uh oh!

robin850 commented Oct 20, 2014

Uh oh!

ryan2johnson9 commented Oct 20, 2014

Uh oh!

bf4 commented Nov 3, 2014

Uh oh!

bf4 commented Nov 3, 2014

Uh oh!

bf4 Nov 4, 2014

Choose a reason for hiding this comment

Uh oh!

robin850 commented Jan 1, 2015

Uh oh!

bf4 commented Feb 9, 2015

Uh oh!

bjfish commented Feb 21, 2015

Uh oh!

robin850 commented Feb 24, 2015

Uh oh!

bf4 commented Feb 24, 2015

Uh oh!

Uh oh!

Use composition over inheritance for `PartsList` #782

Use composition over inheritance for `PartsList` #782