socketry / cloudflare

An asynchronous Ruby wrapper for the CloudFlare V4 API.
MIT License
139 stars 88 forks source link

Fixes socketry/cloudflare#72 #73

Closed julienchabanon closed 2 years ago

julienchabanon commented 2 years ago

Description

The PR is meant to remove deprecated style of argument in DNS Record to be compatible with ruby 3.0. The fix is running in production as a local gem in our projet and it fixed the issue. Other features of the gem might still be broken (or not, to be checked) as I only solved DNS record to fix our issue.

Types of Changes

Testing

f-asa commented 2 years ago

before the fix, we would get this while creating a dns entry with the gem on ruby 3.0.2, it is now resolved.

0.3s error: Async::Task [oid=0x13664] [ec=0x13678] [pid=572800] [2021-11-03 16:09:20 -0400] | ArgumentError: wrong number of arguments (given 4, expected 3) | → /usr/local/bundle/gems/cloudflare-4.3.0/lib/cloudflare/dns.rb:78 in create' | lib/dns_manager/cloudflare_api.rb:145 inblock in create_with_ttl' | /usr/local/bundle/gems/async-1.30.1/lib/async/task.rb:260 in `block in make_fiber'

irb(main):002:0>

ioquatix commented 2 years ago

The PR is meant to remove deprecated style of argument in DNS Record to be compatible with ruby 3.0.

Can you explain what is the deprecated style of arguments. **options is well supported by Ruby 3.0. So, I'm not clear on this point.

f-asa commented 2 years ago

notes from Julien in our internal ticket

After following the white rabbit, I found that the cloudflare gem does not support ruby 3.X.

In there: https://github.com/socketry/cloudflare/blob/master/lib/cloudflare/dns.rb#L78

They use a deprecated style of argument, **options, that is not reconized as a valid parameter in ruby 3. More about this here https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/

ruby 3

3.0.1 :002 > User.last.test 1,2,3,{toto: 'tata'} User Load (1.2ms) SELECT users.* FROM users ORDER BY users.id DESC LIMIT 1 /home/maraud/clusterpbx-mgm/app/models/user.rb:117:in `test': wrong number of arguments (given 4, expected 3) (ArgumentError)

ruby 2.7.1

2.7.1 :001 > User.last.test 1,2,3,{toto: 'tata'} User Load (0.6ms) SELECT users.* FROM users ORDER BY users.id DESC LIMIT 1 (irb):1: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call /home/maraud/clusterpbx-mgm/app/models/user.rb:117: warning: The called method `test' is defined here => "marguerite"

ioquatix commented 2 years ago

Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call

Bingo.

So the problem is not the code, but the way you are using it.

https://bloggie.io/@kinopyo/how-to-fix-ruby-2-7-warning-using-the-last-argument-as-keyword-parameters-is-deprecated

What you need to do is add ** to the place where you call the cloudflare code, on the final hash argument.

f-asa commented 2 years ago

interesting, thx!

julienchabanon commented 2 years ago

@ioquatix Thank you. However, I am assuming that most users might pass options as a hash in a variable like so: zone.dns_records.create('A', 'batman', '1.2.3.4', options)

And for those peoples, their code will be broken as a result when upgrading from ruby 2.x to ruby 3.x. For the record, the solution to this issue would be: zone.dns_records.create('A', 'batman', '1.2.3.4', **options)

This is not intuitive to my opinion but we have a workaround at least.

ioquatix commented 2 years ago

It's just how Ruby is.