googleads / google-api-ads-ruby

Ad Manager SOAP API Client Libraries for Ruby
297 stars 229 forks source link

Fixes on pql_statement_utils to support Ruby < 2.4.0 #157

Closed victor95pc closed 5 years ago

victor95pc commented 5 years ago

Hi, every change is described in the commits, Fixnum is required in order to make ruby 2.1, 2.2, 2,3 to work, and the new error is need to avoid errors later when to_statement() is called.

googlebot commented 5 years ago

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers
victor95pc commented 5 years ago

I signed it!

googlebot commented 5 years ago

CLAs look good, thanks!

donovanfm commented 5 years ago

Hi, Victor. Thanks for this PR! Sorry it took me so long to get to it. Now that I'm working on this I will be much quicker to respond.

My concern is that Fixnum is deprecated as of Ruby 2.4, so I would prefer to add this as a comment so that it's there in case anyone needs it. We want to support users on <2.4, but we don't want to create deprecation warnings for other users and potentially break users when we support newer version of Ruby.

Is that an acceptable compromise?

victor95pc commented 5 years ago

Added condition to check if the ruby version is less than 2.4.0 if so it gonna use Fixnum otherwise it will use Integer.

donovanfm commented 5 years ago

Thank you for the updated solution! I'll test this out and get back to you soon.

donovanfm commented 5 years ago

Hi, Victor. I've refactored some code internally in a way that should fix this issue without needing to check the Ruby version at all. The change will use the is_a? method instead of comparing the class name itself, which removes the need for multiple entries in the VALUE_TYPES hash that point to the same xsi_type value. Please let me know if you have any concerns about that approach addressing your issue. And thanks for bringing this to our attention in the first place. :smiley:

victor95pc commented 5 years ago

Great, way better solution

victor95pc commented 5 years ago

When you push it can you add a comment here?

donovanfm commented 5 years ago

Hi, Victor. I bundled this change into today's release. Can you verify that this resolves the issue on your end, and then we can close this PR.

victor95pc commented 5 years ago

Our team managed to upgrade to ruby 2.5.3, so I can't really test on production, but locally it seems to be working just fine for both ruby versions(2.5.3 and 2.3.1)

donovanfm commented 5 years ago

That's great, Victor. I'll go ahead and close this PR. Thanks for bringing this issue to our attention and going to the trouble to test it!