clowne-rb / clowne

A flexible gem for cloning models
https://clowne.evilmartians.io
MIT License
316 stars 18 forks source link

Ruby 3.0 compatibility #58

Closed pomartel closed 3 years ago

pomartel commented 3 years ago

I started working on Ruby 3 compatibility to fix issue #57 and as I thought it's mostly about fixing positional and keyword arguments.

I got almost all the tests to pass except when related to these two block declarations:

# 1st declaration
finalize do |_source, record, params|
end

# 2nd declaration
finalize do |_source, record, email:, **|
end

I cannot figure out how to support both these declarations with Ruby 3 and keep backward compatibility with prior versions.

The second declaration will not work with Ruby 3 as it is. It can be fixed by adding a double splat operator (**) to the params hash when calling the finalize block:

module Clowne
  class Resolvers
    module Finalize # :nodoc: all
      def self.call(source, record, declaration, params:, **_options)
        declaration.block.call(source, record, **params)
        record
      end
    end
  end
end

But then this breaks the first declaration... I feel like I'm almost there. I hope you can help me figure out the rest!

ssnickolay commented 3 years ago

Impressive work! I'll look at this in the coming days.

ssnickolay commented 3 years ago

Hi @pomartel! I looked at the issue, and it seems you are right: we had to change finalize.rb to use declaration.block.call(source, record, **params) instead of declaration.block.call(source, record, params), and yes, then it brakes many specs like

https://github.com/clowne-rb/clowne/blob/57f195b2dc7969e85ff187144eda25dcc8741444/spec/clowne/adapters/active_record/associations/has_and_belongs_to_many_spec.rb#L43-L45

The problem here in specs itself - we need to change them to work with Ruby3 kwarg properly:

- finalize do |_source, record, params|
+ finalize do |_source, record, **params|
  record.value += params.fetch(:suffix, "-2")
 end

It would be awesome to check & fix docs params usage too.

P.S. not sure how you ran specs because I had to change Gemfile and gemspec a bit more then you:

--- a/Gemfile
+++ b/Gemfile

-gem "activerecord", "~> 5.2"
+gem "activerecord", ">= 6.0", "< 6.2.0"

diff --git a/clowne.gemspec b/clowne.gemspec
index 2c69ad7..558cbcb 100644
--- a/clowne.gemspec
+++ b/clowne.gemspec
@@ -28,7 +28,7 @@ Gem::Specification.new do |spec|
-  spec.add_development_dependency 'factory_bot', '~> 4.8'
+  spec.add_development_dependency 'factory_bot', '~> 5'
pomartel commented 3 years ago

Oh great, I also thought it would be impossible to keep both keyword args and the hash object for the finalize params but I wasn't sure which one of the two you preferred.

I updated my pull request accordingly. Let me know if there is anything missing and thanks for reviewing the PR.

ssnickolay commented 3 years ago

@pomartel we almost there! There is only one think - with current changes we will drop Ruby 2.7- (2.6, 2.5), but it too early for this. Let's tweek a bit the code to be backward compatible with old Ruby versions. @palkan prepared & tested the changes below on 2.6 and 3.0 and it should works on both versions:

diff --git i/lib/clowne/declarations/after_clone.rb w/lib/clowne/declarations/after_clone.rb
index 202e706..0be7b2c 100644
--- i/lib/clowne/declarations/after_clone.rb
+++ w/lib/clowne/declarations/after_clone.rb
@@ -5,7 +5,7 @@ module Clowne
     class AfterClone < Base # :nodoc: all
       attr_reader :block
-      def initialize(&block)
+      def initialize(*, &block)
         raise ArgumentError, "Block is required for after_clone" unless block
         @block = block
diff --git i/lib/clowne/declarations/after_persist.rb w/lib/clowne/declarations/after_persist.rb
index c6904b6..650093d 100644
--- i/lib/clowne/declarations/after_persist.rb
+++ w/lib/clowne/declarations/after_persist.rb
@@ -5,7 +5,7 @@ module Clowne
     class AfterPersist < Base # :nodoc: all
       attr_reader :block
-      def initialize(&block)
+      def initialize(*, &block)
         raise ArgumentError, "Block is required for after_persist" unless block
         @block = block
diff --git i/lib/clowne/declarations/exclude_association.rb w/lib/clowne/declarations/exclude_association.rb
index 7bc85a5..ab61a2b 100644
--- i/lib/clowne/declarations/exclude_association.rb
+++ w/lib/clowne/declarations/exclude_association.rb
@@ -5,7 +5,7 @@ module Clowne
     class ExcludeAssociation < Base # :nodoc: all
       attr_accessor :name
-      def initialize(name)
+      def initialize(name, **)
         @name = name.to_sym
       end
diff --git i/lib/clowne/declarations/finalize.rb w/lib/clowne/declarations/finalize.rb
index 4d39a10..24a3b1f 100644
--- i/lib/clowne/declarations/finalize.rb
+++ w/lib/clowne/declarations/finalize.rb
@@ -5,7 +5,7 @@ module Clowne
     class Finalize < Base # :nodoc: all
       attr_reader :block
-      def initialize(&block)
+      def initialize(*, &block)
         raise ArgumentError, "Block is required for finalize" unless block
         @block = block
diff --git i/lib/clowne/declarations/init_as.rb w/lib/clowne/declarations/init_as.rb
index 87abadf..b8b20d1 100644
--- i/lib/clowne/declarations/init_as.rb
+++ w/lib/clowne/declarations/init_as.rb
@@ -5,7 +5,7 @@ module Clowne
     class InitAs < Base # :nodoc: all
       attr_reader :block
-      def initialize(&block)
+      def initialize(*, &block)
         raise ArgumentError, "Block is required for init_as" unless block
         @block = block
diff --git i/lib/clowne/declarations/nullify.rb w/lib/clowne/declarations/nullify.rb
index cef57fb..76e0573 100644
--- i/lib/clowne/declarations/nullify.rb
+++ w/lib/clowne/declarations/nullify.rb
@@ -5,7 +5,7 @@ module Clowne
     class Nullify < Base # :nodoc: all
       attr_reader :attributes
-      def initialize(*attributes)
+      def initialize(*attributes, **)
         raise ArgumentError, "At least one attribute required" if attributes.empty?
         @attributes = attributes
pomartel commented 3 years ago

Thank you for your guidance. I added the changes you suggested and ran the specs with Ruby 2.6.6, 2.7.2 and 3.0.0. It's all green! 👍

ssnickolay commented 3 years ago

@pomartel thanks a lot for your help! I'll release the new gem version this\next week after migrating from TravisCI to Github Actions.