Skip to content
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

Ruby 3 compatibility #677

Open
swiknaba opened this issue Mar 2, 2021 · 9 comments
Open

Ruby 3 compatibility #677

swiknaba opened this issue Mar 2, 2021 · 9 comments

Comments

@swiknaba
Copy link

swiknaba commented Mar 2, 2021

Have there been any efforts already on getting this gem ready for Ruby 3?

this kind of magic: https://github.com/DatabaseCleaner/database_cleaner/blob/master/lib/database_cleaner/cleaner.rb#L43

    def strategy=(args)
      strategy, *strategy_args = args

won't work in Ruby 3, I've seen quite some usage of this pattern.

Also, the redis and the active_record sub-gems use kwargs vs. options-hashes differently, see:

options hash: https://github.com/DatabaseCleaner/database_cleaner-active_record/blob/master/lib/database_cleaner/active_record/truncation.rb#L8

class Truncation < Base
  def initialize(opts={})

kwargs: https://github.com/DatabaseCleaner/database_cleaner-redis/blob/master/lib/database_cleaner/redis/deletion.rb#L6

    class Deletion < Strategy
      def initialize only: [], except: []

which will make it harder to upgrade the database gem family.

So probably as a first step, we should harmonize all sub-gems to use the same way of passing options around, either as a hash or using keyword arguments. From there we can tinker further.

I've tried it quickly locally and run into too many issues to push a draft PR. I might find more time on one of the next weekends though. -> hence the question, if there is already any PR/fork that started working on Ruby 3 compatibility.

As always, thanks for the gems 🙂

=> Specs are passing for Postgresql, MySQL, SQLlite locally, however, when I use

DatabaseCleaner.strategy = :deletion, { except: %i[not_you also_not_you blubb] }

in one of my projects, strategy= fails with the Ruby 3 kwargs/hash error.

@swiknaba
Copy link
Author

swiknaba commented Mar 8, 2021

@botandrose in case one would start working on this, any preference from the maintainers for either kwargs or options-hashes? I think, using hashes gives more flexibility regarding the DSL, since the gem is broken into multiple gems, and each gem might want to add or omit certain options/key-words. On the other hand, being explicit makes it easier to follow the codebase if you are not familiar with it. From my short look at two of the gems (Redis, active-record), there are not that many options, thus being explicit could be the way to go.

@etagwerker
Copy link
Member

@swiknaba Not sure about Micah's preference, but I would prefer the options-hash way.

So instead of doing this:

    def strategy=(args)
      strategy, *strategy_args = args

I would be interested in it doing this:

    def strategy=(strategy, strategy_args = {})
      ...

I think that would be the least disruptive change and it wouldn't require people using database_cleaner to change their implementations.

@botandrose
Copy link
Contributor

@swiknaba @etagwerker Yes, options-hash sounds good to me!

@botandrose
Copy link
Contributor

@swiknaba @etagwerker I took a look at this today, and I learned something new about Ruby! It looks like the way Ruby handles multiple arguments to setter methods ending in = is a bit of syntax sugar magic. Specifically, it will always first convert the multiple arguments to an array, and then passes that array as a single argument to the method. The only way I could find to bypass this magic is by using send:

DatabaseCleaner.send(:strategy=, :truncation, { only: "users" })

Therefore, in order to keep the same API, we appear to be stuck with the def strategy=(args); strategy, *strategy_args = args; ... pattern. @swiknaba You say in your original post that this won't work on Ruby 3.0, but the tests are all passing, and I wasn't able to reproduce the failure locally. Can you explain what you mean? At present, this implementation doesn't seem to be a problem.

I do agree that we should normalize options hashes vs kwargs across the DatabaseCleaner gem family.

@zedtux
Copy link

zedtux commented Sep 17, 2022

Ruby 3.0 is out since almost 2 years now, when could we expect this gem to be adapted for Ruby 3 ?

@botandrose which branch has that fix ? If I can help by testing it, I would be very happy to do so.

@swiknaba
Copy link
Author

swiknaba commented Sep 17, 2022

@botandrose indeed, tests are all passing, however when I used

DatabaseCleaner.strategy = :deletion, { except: %i[not_you also_not_you blubb] }

in my codebase, then RSpec failed with a syntax error.

It seems there was a workaround found: DatabaseCleaner/database_cleaner-mongoid#16 (comment)

I haven't tested that yet, though.

@zedtux
Copy link

zedtux commented Sep 24, 2022

In the case that helps, here is the deprecation warning from Ruby 2.7 :

/usr/local/bundle/gems/database_cleaner-core-2.0.1/lib/database_cleaner/cleaner.rb:85: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call

@niborg
Copy link

niborg commented Jan 10, 2023

Even though CI seems to pass, for me running Ruby 3.0.3 against v2.01 locally raises two errors:

Screenshot 2023-01-09 at 4 27 06 PM

In any event, I ran into the same issue as others, and circumvented it by specifying the strategy object explicitly:

DatabaseCleaner[:redis].strategy = DatabaseCleaner::Redis::Deletion.new(only: ["something:*"])

This approach is documented in the ActiveRecord adapter, but isn't present on this specific repository and other adapter repositories.

Is an easy solution to this problem adjusting the documentation to recommend the above approach? Anyone upgrading who runs into this issue will likely revisit the docs and can correct the problem easily. Downside is that it smells like a bit of an abstraction leak. If there is a desire to keep using primitive values to set a strategy, then I think the strategy= should be deprecated in favor of something like set_strategy(strategy, strategy_args = {}), similar to how @etagwerker suggested.

@mopp
Copy link

mopp commented Dec 18, 2023

I faced the same issue on Redis adapter and I would be happy if this problem is resolved 👋

Now I resolve the error by the way described at #677 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants