Normalize == / eql? aliasing across value-like classes#2715
Merged
Conversation
83804c7 to
3134edd
Compare
Danger ReportNo issues found. |
3134edd to
3170e86
Compare
Several classes defined `==` without aliasing it to `eql?`, leaving hash-table membership (`include?`, `Set`, `Hash` keys) inconsistent with their custom equality. Normalize every value-like class to the same shape: `def ==(other); ...; end; alias eql? ==`. - middleware/stack.rb (Stack::Middleware): add `alias eql? ==`. - serve_stream/stream_response.rb: same. - serve_stream/file_body.rb: same. - util/media_type.rb: collapse the `def ==(other); eql?(other); end` trampoline + separate `def eql?` into a single `==` body with `alias eql? ==`. Same behaviour, less indirection. - exceptions/error_response.rb: same collapse. - namespace.rb: flip from `def eql? ... alias == eql?` to `def == ... alias eql? ==` so the project follows one direction consistently. `endpoint.rb` already used the canonical pattern; left untouched. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3170e86 to
b058fce
Compare
dblock
approved these changes
May 14, 2026
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
Endpointalready followed the canonical Ruby pattern (def ==thenalias eql? ==), but several value-like classes either defined==without aneql?alias (so they wouldn't compare correctly as hash keys / set members), or defined the two as separate methods that drifted. Normalizing every one to the same shape:Equality changes
Grape::Middleware::Stack::Middleware— addalias eql? ==.Grape::ServeStream::StreamResponse— same.Grape::ServeStream::FileBody— same.Grape::Util::MediaType— collapse thedef ==(other); eql?(other); endtrampoline + separatedef eql?into a single==body withalias eql? ==. Same behaviour, one fewer dispatch.Grape::Exceptions::ErrorResponse— same collapse.Grape::Namespace— flip fromdef eql? ... alias == eql?todef == ... alias eql? ==so the codebase is consistent on one direction.Grape::Util::InheritableSetting— adddef ==(other)(compares viato_hashinternally) andalias eql? ==. The class previously had no equality at all, soEndpoint#==had to callinheritable_setting.to_hash == other.inheritable_setting.to_hashto compare. With this in place:Grape::Endpoint#==simplifies frominheritable_setting.to_hash == other.inheritable_setting.to_hashto plaininheritable_setting == other.inheritable_setting.Grape::Endpoint's own==was already canonical and remains so.Encapsulation cleanup in
InheritableSettingWhile in the file, tightened the public surface — none of
route,api_class,namespace,namespace_inheritable,namespace_stackable,namespace_reverse_stackable,parent,point_in_time_copiesis written from outside the class anywhere inlib/orspec/. They wereattr_accessorbut only ever neededattr_reader.attr_accessor→attr_readerfor all eight.initializeandinherit_fromnow write to@ivardirectly instead of the awkwardself.foo = …setter syntax (matchingroute_end's existing@route = {}).point_in_time_copypreviously mutated the new instance by calling its public setters from the outside. Replaced that with a singleprotectedcopy_state_from(source)helper onInheritableSettingitself — the new instance pulls its state fromsourcefrom inside the class, with no public writers required.Why it matters
Without the alias,
[obj1].include?(obj2)andSet#include?useeql?(which falls back toequal?/ object identity) instead of the custom==, producing surprisingfalseresults when two value-equal instances are compared by hash-based collections. TheInheritableSettingencapsulation cleanup is a related-but-separate readability win — the class no longer leaks writable state, and internal mutations go through the canonical@ivar = …form.Test plan
bundle exec rspec— 2292 examples, 0 failures🤖 Generated with Claude Code