Fix ActiveRecord STI format lookup and guard __use__ cycles - #173
Open
eitoball wants to merge 4 commits into
Open
Fix ActiveRecord STI format lookup and guard __use__ cycles#173eitoball wants to merge 4 commits into
eitoball wants to merge 4 commits into
Conversation
Replace the class_attribute + inherited-hook dup snapshot with a per-class instance variable and a call-time walk up the superclass chain, so a subclass's comma_formats always reflects the current state of its ancestors instead of a stale copy taken when the subclass was first defined.
Cat#to_comma correctly returns Super-Kitty now that comma_formats walks the class hierarchy at call time instead of relying on a snapshot taken when the subclass was defined.
… stack overflow Track the chain of styles currently being expanded and raise a dedicated error the moment __use__ is asked to re-enter a style already on the stack, instead of recursing until the interpreter's stack overflows.
Use the symbol-to-proc form for the value transform and mark the STI describe block as an accepted long block, matching the existing convention elsewhere in this file.
There was a problem hiding this comment.
Pull request overview
This PR fixes two correctness gaps in Comma’s DSL execution: (1) STI subclasses now resolve comma formats from the live class hierarchy at call time (instead of an inherited snapshot), and (2) __use__ style composition is protected against circular references by raising a dedicated Comma::CircularStyleReference error.
Changes:
- Reworked format storage/lookup to merge per-class format definitions across the superclass chain at call time (fixes STI/reopened-superclass cases).
- Added circular
__use__detection with a new exception type and expanded specs to cover both indirect and direct cycles. - Updated ActiveRecord STI spec by removing the now-stale FIXME comment (test expectation remains the same).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
lib/comma/object.rb |
Replaces inherited snapshotting with call-time hierarchy merge for comma_formats. |
lib/comma/extractor.rb |
Adds CircularStyleReference and a style stack to detect __use__ cycles. |
spec/comma/comma_spec.rb |
Adds regression tests for reopened superclass formats and circular __use__ chains. |
spec/comma/rails/active_record_spec.rb |
Removes outdated comment now that STI format lookup is fixed. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+30
to
+32
| @style_stack.push(style) | ||
| instance_eval(&@formats[style]) | ||
| @style_stack.pop |
Comment on lines
49
to
52
| def extract_with(extractor_class, style = :default) | ||
| raise_unless_style_exists(style) | ||
| extractor_class.new(self, style, self.comma_formats).results | ||
| extractor_class.new(self, style, comma_formats).results | ||
| end |
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.
What
Resolves
comma_formatsfrom the class hierarchy at call time instead of a stale snapshot, and guardsComma::Extractor#__use__against circular style references.Why
Closes #161.
Object.comma_formatsusedclass_attributeplus aninherited-hook shallowdup, so a subclass's format table was a snapshot frozen at the moment the subclass was defined — any later change to the superclass'scommablocks (e.g. reopening the class) silently failed to reach subclasses, including ActiveRecord STI subclasses. Separately,__use__re-evaluated named styles with no cycle detection, so a circular:auses:b,:buses:areference would stack-overflow instead of raising a clear error.Changes
lib/comma/object.rb: dropclass_attribute :comma_formatsand theinheriteddup hook; store each class's own DSL entries in a private, never-auto-inherited@own_comma_formatsivar, and resolve the effectivecomma_formatsby walkingself→superclass→ ... at call time, merging so a subclass's own style entries override the same-named parent style while other styles still fall through. Restore an instance-levelcomma_formatsreader (self.class.comma_formats) to preserve the pre-existing public behavior for receivers that are themselvesClassobjects (see the#94regression spec).lib/comma/extractor.rb: addComma::CircularStyleReference < StandardErrorand a@style_stackthat__use__pushes/pops around each nested expansion, raising immediately if a style already on the stack is requested again.spec/comma/comma_spec.rb: add a regression spec proving a superclasscommablock reopened after a subclass is defined is still picked up (ChildClassNoComma-style, but redefining the parent block post-hoc), and two new specs for a two-style cycle and a direct self-reference, both expectingComma::CircularStyleReference.spec/comma/rails/active_record_spec.rb: remove the stale# FIXMEcomment on theCat#to_commaSTI example — it now passes.Verified against the base spec suite plus the
active_record-only appraisal gemfiles for 6.0.6, 6.1.7.6, 7.0.8, and 7.1.3 (all green), and rubocop on the changed files (no offenses).