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

Support Ruby 2.8 keyword arguments for google-cloud-storage #4884

Merged
merged 2 commits into from
Mar 11, 2020
Merged

Support Ruby 2.8 keyword arguments for google-cloud-storage #4884

merged 2 commits into from
Mar 11, 2020

Conversation

yahonda
Copy link
Contributor

@yahonda yahonda commented Feb 27, 2020

This pull request supports Ruby 2.8 keyword arguments for google-cloud-storage

Fully separate positional arguments and keyword arguments support is required to use google-cloud-storage gem with currently developed Ruby 2.8.0-dev.

Note

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 27, 2020
@quartzmo quartzmo added the api: storage Issues related to the Cloud Storage API. label Feb 27, 2020
@quartzmo
Copy link
Member

Hi @yahonda,

Thank you for opening this PR. Do you have an urgent need for it to be merged?

@yahonda
Copy link
Contributor Author

yahonda commented Feb 28, 2020

Thanks for the reply.

Let me update why I need this change, No urgency but it is important to prepare Ruby 3 and avoid warnings for Ruby 2.7 user.

google-cloud-storage gem is required for one of the major components of Ruby on Rails, called Active Storage. Ruby on Rails CI is running also with Ruby 2.8.0-dev and CI of Active Storage is failing likely because google-cloud-storage does not support the Ruby 3 keyword arguments yet.

  • Active Storage CI with Ruby 2.8.0-dev - raise ArgumentError: wrong number of arguments

https://buildkite.com/rails/rails/builds/67309#0d0e7865-29ec-49a4-99cb-cdc7b7ea596f/942-1266

ArgumentError: wrong number of arguments (given 4, expected 2..3)

Also, Ruby 2.7 shows warnings if the code is not ready for Ruby 3 keyword arguments.

  • Active Storage CI with Ruby 2.7.0 - show warnings about the keyword arguments:

https://buildkite.com/rails/rails/builds/67301#4b646278-fdf1-4e12-86bc-07ea4c2768c0/1008-1275

/usr/local/bundle/gems/google-cloud-storage-1.25.1/lib/google/cloud/storage/bucket.rb:1272: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call

I'd like to make Rails CI green as much as possible to prepare Ruby 3. Thanks.

@quartzmo
Copy link
Member

Thanks for the detailed explanation. This makes perfect sense, and we will try to help solve your CI build problem as fast as we can. We are hoping that gRPC will be updated soon, so that possibly we can avoid the Bundler hacks in this PR. In addition, I am thinking that some of these conversions to double splats can just be converted to definite keyword args instead.

@quartzmo
Copy link
Member

This library was originally written to support Ruby 1.9 (no longer supported), so there are places where options was used as a convenience and where it can now be replaced with a simple list of keyword args. However, there are also some options hashes that are quite large.

Copy link
Member

@quartzmo quartzmo left a comment

Choose a reason for hiding this comment

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

If you can remove the Bundler hacks (and wait for grpc to be updated, hopefully it won't be long), I am ready to approve this PR now with the following four small changes to use keyword args inline.

google-cloud-storage/lib/google/cloud/storage/file/list.rb Outdated Show resolved Hide resolved
google-cloud-storage/lib/google/cloud/storage/bucket.rb Outdated Show resolved Hide resolved
google-cloud-storage/lib/google/cloud/storage/bucket.rb Outdated Show resolved Hide resolved
google-cloud-storage/lib/google/cloud/storage/bucket.rb Outdated Show resolved Hide resolved
Copy link
Member

@quartzmo quartzmo left a comment

Choose a reason for hiding this comment

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

I found a few rubocop violations. Run rake rubocop or rake ci to check.

google-cloud-storage/lib/google/cloud/storage/bucket.rb Outdated Show resolved Hide resolved
google-cloud-storage/lib/google/cloud/storage/file.rb Outdated Show resolved Hide resolved
google-cloud-storage/lib/google/cloud/storage/service.rb Outdated Show resolved Hide resolved
@quartzmo
Copy link
Member

quartzmo commented Mar 2, 2020

Here's a commit for my suggestions that you are welcome to include in this PR if it is helpful.

@quartzmo quartzmo self-assigned this Mar 2, 2020
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Mar 2, 2020
@yahonda
Copy link
Contributor Author

yahonda commented Mar 2, 2020

Thanks for the detailed review. I agree with your commit then cherry-pick quartzmo@76b815f and pushed to this pull request.

@quartzmo
Copy link
Member

quartzmo commented Mar 2, 2020

@googlebot I consent.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Mar 2, 2020
@quartzmo
Copy link
Member

quartzmo commented Mar 2, 2020

@yahonda What do you think about removing the Bundler hacks in favor of waiting for a grpc update?

yahonda and others added 2 commits March 2, 2020 20:37
* To support "Fully separate positional arguments and keyword arguments"
ruby/ruby#2794

* `test/google/cloud/storage_test.rb` failure will be addressed
once the newer version of minitest is released including the pull
request below:

"If a callable mock includes kwargs, Ruby 2.7 warns"
minitest/minitest#824
@yahonda
Copy link
Contributor Author

yahonda commented Mar 2, 2020

Agreed. I have removed the bundler hack and squashed commits.

@quartzmo quartzmo added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 2, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 2, 2020
@quartzmo quartzmo requested a review from dazuma March 2, 2020 15:43
Copy link
Member

@quartzmo quartzmo left a comment

Choose a reason for hiding this comment

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

@yahonda Thank you very much for helping us with this!

quartzmo added a commit that referenced this pull request Mar 4, 2020
To prepare Ruby 3 keyword arguments support, there are some other gems also need update. `rake` is one of them.

* Rake 13.0.0 supports Ruby 3 keyword arguments by ruby/rake#326
* Rake 13 drops Ruby 2.1 or older support by ruby/rake#325
* Gems under `google-cloud-ruby` repository only supports Ruby 2.4 or higher then dropping Ruby 2.1 support should not be an issue.

refs: #4884
pr: #4897
@dazuma dazuma merged commit 77bdc4a into googleapis:master Mar 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the Cloud Storage API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants