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.

Sign up for GitHub

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

Treat Middleware::Base#initialize options as Hash. #2064

Merged
merged 3 commits into from
May 29, 2020
Merged

Treat Middleware::Base#initialize options as Hash. #2064

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aebb043
Treat Middleware::Base#initialize options as Hash.
skarger May 28, 2020
94725a5
Add PR link to CHANGELOG
skarger May 29, 2020
ddbaa81
Fix CHANGELOG entry
skarger May 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#### Fixes

* Your contribution here.
* [#2064](https://github.com/ruby-grape/grape/pull/2064): Fix Ruby 2.7 deprecation warning in `Grape::Middleware::Base#initialize` - [@skarger](https://github.com/skarger).

### 1.3.3 (2020/05/23)

Expand Down
4 changes: 2 additions & 2 deletions lib/grape/middleware/base.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@ class Base

# @param [Rack Application] app The standard argument for a Rack middleware.
# @param [Hash] options A hash of options, simply stored for use by subclasses.
def initialize(app, **options)
def initialize(app, options = {})
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the backwards-compatible workaround solution would be:

def initialize(app, options = nil)
  @app = app
  @options = default_options.dup
  @options.update(options) if options.is_a?(Hash)
  @app_response = nil
end

This retains the optional argument character of options while resolving the deprecation warning.

Copy link
Member

Choose a reason for hiding this comment

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

And do we need it to be backwards compatible here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Defaulting the options parameter to an empty hash is backwards compatible. When declaring the parameter with **options, the options variable inside the method will default to an empty hash.

See this example, run with Ruby 2.7.

def example_method(positional, **options)
  puts "  positional: #{positional}"
  puts "  options class: #{options.class}"
  puts "  options: #{options}\n"
end

Given positional argument only:
example_method("arg1")
  positional: arg1
  options class: Hash
  options: {}

Given positional argument followed by hash argument:
example_method("arg1", a_key: "a value")
  positional: arg1
  options class: Hash
  options: {:a_key=>"a value"}

Like the existing initialize method in Grape::Middleware::Base, the fact that example_method has options with the ** in front declares it a keyword arguments, not as a (positional) hash argument. However the logic inside Grape::Middleware::Base#initialize treats options as a hash - it just merges it with default_options.

When passing a hash as a positional argument, Ruby 2.7 prints the deprecation warning because it's treating the given hash as keyword arguments in accordance with the method signature:

Given positional argument followed by positional hash:
example_method("arg1", { a_key: "a value" })
test.rb:15: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
test.rb:1: warning: The called method `example_method' is defined here
  positional: arg1
  options class: Hash
  options: {:a_key=>"a value"}

Changing the signature to def example_method(positional, options = {}) makes the deprecation warning go away, but otherwise matches the same behavior for all of these examples. This is what I've done in this PR.

@app = app
@options = default_options.merge(**options)
@options = default_options.merge(options)
@app_response = nil
end

Expand Down