Skip to content

Revisit Grape's middlewares default options #2561

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

Merged

Conversation

ericproulx
Copy link
Contributor

@ericproulx ericproulx commented May 1, 2025

See copilot's comment but it should:

  • reduce Hash allocations since default_options have been replaced by frozen constants
  • reduce a little bit default_options size since nil values have been removed in favor of keeping actual default.
  • add clarity about the default options per middleware

`default_options` have been transformed has const
Grape::Middleware::Helpers has been removed
@ericproulx ericproulx requested a review from Copilot May 1, 2025 11:48
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR revisits how default options are handled in Grape’s middleware by updating the option merging strategy and shifting from method‐based defaults to constant-based defaults, while also updating the initialization across several middlewares to use keyword arguments.

  • Updated middleware initializations to use keyword arguments (double-splat)
  • Replaced default_options methods with DEFAULT_OPTIONS constants where applicable
  • Removed the Helpers module from lib/grape/middleware/helpers.rb

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.

Show a summary per file
File Description
spec/grape/middleware/versioner/*_spec.rb Updated subject initializations to use keyword arguments
spec/grape/middleware/formatter_spec.rb Updated formatter configuration assignment to overwrite the formatters hash entirely
spec/grape/middleware/exception_spec.rb & error_spec.rb Updated Error middleware to use keyword arguments
spec/grape/middleware/base_spec.rb Updated default options test to verify constant DEFAULT_OPTIONS usage
lib/grape/middleware/versioner/base.rb Replaced method-based default options with a DEFAULT_OPTIONS constant
lib/grape/middleware/helpers.rb Removed the now-unused Helpers module
lib/grape/middleware/formatter.rb Replaced default_options method with a DEFAULT_OPTIONS constant with a simplified configuration
lib/grape/middleware/error.rb Updated Error middleware to use keyword arguments and constant DEFAULT_OPTIONS
lib/grape/middleware/base.rb Adjusted initialization to merge default options via a helper method
lib/grape/middleware/auth/base.rb Updated Auth middleware initialization and streamlined the _call method logic
Comments suppressed due to low confidence (2)

lib/grape/middleware/formatter.rb:6

  • Removing the default empty initializations for 'formatters' and 'parsers' from the DEFAULT_OPTIONS may lead to issues if downstream code relies on their existence. Consider retaining these keys with empty hashes or ensuring downstream code handles their absence.
DEFAULT_OPTIONS = { default_format: :txt }.freeze

spec/grape/middleware/formatter_spec.rb:239

  • Replacing the in-place update of the formatters hash with a full assignment might remove other formatters previously set. Please confirm that this behavior is intentional and that any dependent code is adjusted accordingly.
subject.options[:formatters] = { json: ->(_obj, _env) { 'CUSTOM JSON FORMAT' } }

@ericproulx ericproulx requested a review from dblock May 1, 2025 18:32
@ericproulx ericproulx force-pushed the revisit_middleware_default_options branch from 784731f to 40e6f7b Compare May 1, 2025 18:37
@ericproulx ericproulx marked this pull request as ready for review May 1, 2025 18:38
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

The caller's hash should be left alone even if internal.

@ericproulx
Copy link
Contributor Author

@dblock is changing Grape::Middleware::Base signature like I did considered has a breaking change ?

@dblock
Copy link
Member

dblock commented May 3, 2025

@dblock is changing Grape::Middleware::Base signature like I did considered has a breaking change ?

You know, I think it may be, since we do mention Base in at least one example in README.md. But I don't think we mention the initializer. WDYT?

@ericproulx
Copy link
Contributor Author

@dblock is changing Grape::Middleware::Base signature like I did considered has a breaking change ?

You know, I think it may be, since we do mention Base in at least one example in README.md. But I don't think we mention the initializer. WDYT?

There's a least an upgrading notes and I think its sufficient.

@dblock dblock merged commit 09e1bf5 into ruby-grape:master May 4, 2025
63 checks passed
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

Successfully merging this pull request may close these issues.

2 participants