Repository navigation
Don't require a custom strategy to implement #to_sym - #1839
Open
IslamElsayed wants to merge 2 commits into
Open
IslamElsayed wants to merge 2 commits into
IslamElsayed wants to merge 2 commits into
Conversation
With `use_parent_strategy` on, building a factory that has an association
called `@build_strategy.to_sym` unconditionally to decide which strategy
the association should use. A strategy only has to implement `#result` and
`#association`; `#to_sym` is something the five built-in strategies happen
to have, so any custom strategy without it raised:
NoMethodError: undefined method `to_sym' for an instance of #<Class:...>
Fall back to the strategy's class, which is what @composerinteralia
suggested on thoughtbot#1536. `Strategy.lookup_strategy` already returns a Class
unchanged, so the association is built with the same custom strategy as
its parent, which is what `use_parent_strategy` means.
The condition is inlined rather than extracted into a method because
`Evaluator` defines a factory's attributes as singleton methods on itself,
so a new instance method could shadow an attribute of the same name.
Closes thoughtbot#1536
IslamElsayed
requested review from
neilvcarvalho and
vburzynski
as code owners
September 19, 2026 21:28
This branch has not been deployed
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.
Fixes #1536.
The bug
With
use_parent_strategyon (the default), building a factory that has an association calls@build_strategy.to_symunconditionally:A strategy only has to implement
#resultand#association—#to_symis something the five built-in strategies happen to have. Any custom strategy without it raises:@composerinteralia called this an unintentional breaking change on the issue and wrote out the fix; it was never implemented, and the only PR on the thread (#1537) was docs-only.
The fix
His suggestion, falling back to the strategy's class. That works because
Strategy.lookup_strategyalready returns aClassunchanged:so the association is built with the same custom strategy as its parent — which is what
use_parent_strategyis asking for.One deliberate deviation: I inlined the condition rather than extracting it into a private method.
Evaluatordefines a factory's attributes as singleton methods on itself, so adding an instance method risks shadowing an attribute that happens to share its name.Testing
An acceptance example in
register_strategies_spec.rbregistering a strategy that implements#resultand#associationbut not#to_sym, and asserting the association is built. Before the fix it fails with exactly theNoMethodErrorfrom the issue.I checked it isn't vacuous by breaking the fix two ways: restoring the unconditional
to_symfails it, and returning:createfrom the fallback instead of the class also fails it — so it pins that the association uses the custom strategy, not merely that nothing raises.Full suite: 775 examples, 0 failures.
standardrbclean on both files. Run under Ruby 3.3.5 rather than the 4.0.1 in.tool-versions, since the gemspec allows>= 3.0.0.I have not added the deprecation @composerinteralia mentioned for eventually removing the fallback — happy to if you want it in the same PR.
Disclosure: I use AI assistance when working on patches, and I check and test everything before submitting.