Skip to content

Remove Grape::Util::InheritableValues, resolving inheritable settings by walking parent#2830

Open
ericproulx wants to merge 2 commits into
masterfrom
remove-inheritable-values
Open

Remove Grape::Util::InheritableValues, resolving inheritable settings by walking parent#2830
ericproulx wants to merge 2 commits into
masterfrom
remove-inheritable-values

Conversation

@ericproulx

Copy link
Copy Markdown
Contributor

Follows #2823, which moved stackable storage onto Grape::Util::InheritableSetting and left StackableValues a read-only view. This does the same for the inheritable scalars, in two commits.

1. Store namespace settings in a plain Hash

InheritableValues layers a scope's own values over an enclosing scope's — but nothing ever gave @namespace an enclosing scope to layer over. #inherit_from assigns inherited_values to @namespace_inheritable only, so @namespace's stayed {} for the object's whole life.

That isn't an oversight: namespace settings are scope-local by design, and spec/grape/dsl/settings_spec.rb:48 pins that a nested namespace must not see the outer one's value. So the layering resolved nothing and the store was a Hash with extra indirection. #namespace_setting reads and writes through #[]/#[]= either way, so the DSL is untouched.

2. Resolve inheritable settings by walking parent

The parent relationship was encoded twice — once as @parent, once as the inherited_values chain — and #inherit_from had to keep them in sync. Two representations of one fact is the shape that produced #2794, where rescue handlers were written to and read from different stores.

A scope now keeps only its own overrides, in a Hash that stays nil until the first write (like @stackable_values), and reads resolve against #parent:

def inheritable(key)
  return @namespace_inheritable[key] if @namespace_inheritable&.key?(key)

  parent&.inheritable(key)
end

with #inheritable? for the presence predicate behind cascade_defined? and #inheritable_values for the merged view #to_hash needs — mirroring #stacked / #stacked_keys. The 39 @namespace_inheritable[...] call sites become inheritable(:key) / set_inheritable(:key, value). InheritableValues then has nothing left to do and is removed, along with its #merge and #delete, which had no caller in lib/ (#merge doubly so — it merged into a throwaway copy and discarded the result).

Behavior preserved

Three invariants carry the risk here, and all three are unchanged:

  • Presence, not truthiness. A scope can override an inherited value with an explicit nil#cascade documents and depends on this, and a ||-based walk would silently break it.
  • The predicate spans the chain. cascade_defined? still reports an assignment made by any enclosing scope, including one that assigned nil.
  • The chain stays live. A value an enclosing scope gains after a nested scope was created is still visible through it, which point_in_time_copy and re-parenting rely on.

Tests

The deleted class's spec is replaced by coverage of those invariants through InheritableSetting's own API — where, notably, they were previously untested: nothing exercised cascade_defined? or the nil-override rule. I checked the new specs earn their place by running them against a naive truthiness-based #inheritable; the nil-override example fails as intended.

Suite green throughout (2458 examples: 2462 − 9 deleted InheritableValues examples + 5 new), and green at commit 1 alone (2462) so the two are independently bisectable. Metrics/ClassLength 395 → 410.

Note on #2828

The inheritable-setting-cheap-equality work adds InheritableValues#== and calls it "the actual blocker, since it defined no equality and every copy is a distinct object". With plain Hashes that method is unnecessary — Hash#== is free — so if this lands first, that branch loses a moving part rather than gaining a conflict.

🤖 Generated with Claude Code

ericproulx and others added 2 commits July 26, 2026 22:11
InheritableValues layers a scope's own values over an enclosing scope's,
but nothing ever gave @namespace an enclosing scope to layer over:
#inherit_from assigns inherited_values only to @namespace_inheritable.
Namespace settings are scope-local by design — a nested namespace must
not see the outer one's value, which DSL::Settings#namespace_setting is
specified to do — so the layering resolved nothing and the store was a
Hash with extra indirection.

Keep it as a Hash. #namespace_setting reads and writes through #[] and
#[]= either way, so the DSL is unaffected.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Each InheritableSetting held an InheritableValues layered over the store
of the scope it inherited from, so the parent relationship was encoded
twice: once as @parent, once as that chain, kept in sync by
#inherit_from. Two representations of one fact is the shape that
produced #2794, where handlers were written to and read from different
stores.

Keep a scope's own overrides in a plain Hash — nil until first write,
like @stackable_values — and resolve reads against #parent, the same
move StackableValues got in #2823. InheritableValues then has nothing
left to do and is removed, along with its unreachable #merge and
#delete.

Resolution is unchanged: reads key off presence rather than truthiness,
so a scope can still override an inherited value with an explicit nil
(#cascade relies on this), #inheritable? still reports an assignment
made anywhere along the chain, and the chain stays live, so a value an
enclosing scope gains later is visible through scopes already nested
inside it. The deleted class's spec is replaced by coverage of those
invariants through InheritableSetting's own API, where they were
previously untested.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@ericproulx
ericproulx force-pushed the remove-inheritable-values branch from 6cdf766 to 3d0d87a Compare July 26, 2026 20:17
@github-actions

github-actions Bot commented Jul 26, 2026

Copy link
Copy Markdown

Danger Report

No issues found.

View run

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