Refactor DSL::Routing#version: guard clause, explicit kwargs, value object#2716
Open
ericproulx wants to merge 1 commit into
Open
Refactor DSL::Routing#version: guard clause, explicit kwargs, value object#2716ericproulx wants to merge 1 commit into
ericproulx wants to merge 1 commit into
Conversation
Danger ReportNo issues found. |
cd36ee1 to
bfe3768
Compare
d10eb26 to
5ac2bf5
Compare
5c4f190 to
9b20ad0
Compare
3 tasks
Three changes that touch the same method: 1. Lift the body out of `if args.any? ... end` into a `return @versions&.last if args.empty?` guard. 2. Replace `**options` with explicit kwargs (`using:`, `cascade:`, `parameter:`, `strict:`, `vendor:`) and bake the defaults into the signature. The per-call `reverse_merge(using: :path)` is gone. 3. Introduce `Grape::DSL::VersionOptions` (built on `Data.define`) as the canonical representation of resolved version options. Threaded end-to-end: - `#version` builds and stores a `VersionOptions` on `inheritable_setting.namespace_inheritable[:version_options]`. - `Path#uses_path_versioning?`, `Endpoint#build_stack`, and `API::Instance#cascade?` read fields via accessors instead of `[:key]`. - The versioner middleware (`Versioner::Base`) now takes a `VersionOptions` directly, not a Hash. `Forwardable.def_delegators` forwards `cascade`/`parameter`/`strict`/`vendor` to it; the four ivars and per-call assignments are dropped. `DEFAULT_OPTIONS` stores a `VersionOptions.new(...)` so direct-mount callers still get safe defaults. Public `.version` DSL surface unchanged — still accepts kwargs the same way; the value object is an internal-only representation. `version_options` is read for exactly these five keys across the codebase: `:using` (`endpoint.rb`, `path.rb`), and `:cascade` / `:parameter` / `:strict` / `:vendor` (`Versioner::Base`, plus `:cascade` in `API::Instance#cascade?`). Any other kwarg passed to `.version` was previously swallowed by the splat with no effect. Spec fixtures updated: - `spec/shared/versioning_examples.rb` (14 sites): `**macro_options` -> `**macro_options.except(:format)`. The `:format` key stays in the macro hash so the test helper (`versioned_helpers.rb`) can still build the `HTTP_ACCEPT` header. - `spec/grape/exceptions/invalid_accept_header_spec.rb` (8 sites): dropped `format: :json` from direct `.version` calls. - `spec/grape/dsl/routing_spec.rb`: assertion now expects a `VersionOptions` instance with the full default set. - `spec/grape/path_spec.rb`: literal `version_options: { using: :path }` hashes replaced with `VersionOptions.new(...)` constructors. - `spec/grape/middleware/versioner/{header,accept_version_header,param}_spec.rb`: fixtures construct `VersionOptions` instances; mid-test `@options[:version_options][:strict] = X` assignments rewritten as `@options[:version_options] = @options[:version_options].with(strict: X)` (Data instances are immutable). No production behaviour change beyond `.version` raising `ArgumentError` for genuinely unknown kwargs (previously silently ignored). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ea89be3 to
526f59f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three changes to
Grape::DSL::Routing#versionplus the call sites and middleware that consume its output:1. Guard clause
if args.any? ... endwrapping a 20-line body →return @versions&.last if args.empty?.2. Explicit kwargs in place of
**optionsDefaults live in the signature instead of being applied later via
reverse_merge+ the versioner's deep-mergedDEFAULT_OPTIONS. The per-callreverse_mergeis gone.3.
Grape::DSL::VersionOptionsvalue object, threaded end-to-endNew file
lib/grape/dsl/version_options.rb:#versionbuilds and stores aVersionOptionsoninheritable_setting.namespace_inheritable[:version_options].Path#uses_path_versioning?,Endpoint#build_stack, andAPI::Instance#cascade?read fields via accessors instead of[:key].Versioner::Base) now takes theVersionOptionsdirectly, not a Hash.Forwardable.def_delegatorsforwardscascade/parameter/strict/vendorto it; the four ivars and per-call assignments in#initializeare dropped.DEFAULT_OPTIONSstoresVersionOptions.new(...)so direct-mount callers still get safe defaults.Public
.versionDSL surface is unchanged — still accepts kwargs the same way; the value object is an internal-only representation.version_optionsis read for exactly these five keys across the codebase, and nothing else::usingendpoint.rb,path.rb:cascadeVersioner::Base,API::Instance#cascade?:parameterVersioner::Base:strictVersioner::Base:vendorVersioner::BaseAny other kwarg passed to
.versionwas previously swallowed by**optionswith no effect — nowArgumentError.Spec fixtures updated
spec/shared/versioning_examples.rb— 14 sites changed**macro_options→**macro_options.except(:format). The:formatkey stays in the macro hash soversioned_helpers.rbcan still build theHTTP_ACCEPTheader.spec/grape/exceptions/invalid_accept_header_spec.rb— droppedformat: :jsonfrom 8 direct.versioncalls.spec/grape/dsl/routing_spec.rb—:version_optionsassertion now expects aVersionOptionsinstance.spec/grape/path_spec.rb— literal hashes →VersionOptions.new(...).spec/grape/middleware/versioner/{header,accept_version_header,param}_spec.rb— fixtures constructVersionOptionsinstances; mid-test@options[:version_options][:strict] = Xmutations rewritten as@options[:version_options] = @options[:version_options].with(strict: X)(Data instances are immutable).Why
format:was in the specs (and why dropping it from.versionis correct)Several specs passed
format: :json(orformat: 'json'inmacro_options) directly to.version. This was test-fixture double-duty, never a real.versionfeature:spec/support/versioned_helpers.rb'sversioned_headersbuilds the requestHTTP_ACCEPTheader from the same hash: for header versioning it producesapplication/vnd.#{vendor}-#{version}+#{format}. So:formatexists for the test helper to construct the request's Accept header.subject.version 'v1', **macro_options) and driving the request (versioned_get '/x', 'v1', macro_options) — so:formatrode into.versionpurely as a side effect of the splat..versionnever had aformat:option. Its value lands innamespace_inheritable[:version_options], and the versioner middleware only ever reads:cascade/:parameter/:strict/:vendorfrom it — nothing readsversion_options[:format]. For header versioning the response format is parsed out of the request Accept header (media_type.format→env['api.format']); the API's serialization format is set independently byformat/default_format. So under the old**optionssplat,format:on.versionwas silently swallowed and inert.The fix preserves intent exactly:
spec/shared/versioning_examples.rb—.versionis called with**macro_options.except(:format)(drops the inert kwarg), butversioned_getstill receives the fullmacro_options, so the request Accept header is stillapplication/vnd.mycompany-v1+json— byte-identical to before. The header-versioning + format-negotiation path is genuinely still exercised.spec/grape/exceptions/invalid_accept_header_spec.rb—format: :jsondropped from direct.versioncalls; those tests build their Accept header inline, so it was equally dead there.Verified: header-versioning shared examples 12/0;
invalid_accept_header_spec.rb+ allspec/grape/middleware/versionerspecs 110/0. Same requests sent, same behaviour — only a silent no-op kwarg on.versionis now (correctly) an explicitArgumentError.Behaviour change
No production behaviour change beyond
.versionraisingArgumentErrorfor genuinely unknown kwargs (previously silently ignored — see theformat:note above for the canonical example).Documented in
UPGRADING.mdunder### Upgrading to >= 3.3→ "versionnow takes explicit keyword arguments", with the recognised-key list and theformat:before/after.Test plan
bundle exec rspec— 2292 examples, 0 failures🤖 Generated with Claude Code