refactor: clean up accessor internals#27
Closed
Koan-Bot wants to merge 1 commit into
Closed
Conversation
Owner
|
@Koan-Bot rebase |
… dispatch) Three focused improvements to the accessor implementation: 1. Move `require mro` to module level — was called inside _all_attributes() on every new() invocation (closes #22) 2. Replace string eval with block eval for role require — the module name is already validated by regex, so eval { require $file } is safer and avoids potential edge cases with string eval (closes #25) 3. Unify hook resolution in setter and builder closures — the rollback logic for _after_* hooks was copy-pasted in two branches (class method vs role chain). Now resolves the hook coderef first, then calls it once with a single rollback path (closes #26) No behavior change. All 311 existing tests pass. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Collaborator
Author
Rebase: refactor: clean up accessor internalsBranch Diff: 1 file changed, 23 insertions(+), 23 deletions(-) Review feedback was analyzed and applied. Actions
CICI will be checked asynchronously. Automated by Kōan |
Koan-Bot
force-pushed
the
koan.atoomic/refactor-accessor-internals
branch
from
a80e837 to
615768f
Compare
Collaborator
Author
|
This PR now has merge conflicts after recent master changes (hook cache, hashref constructor, etc.). The refactoring scope has also been partially superseded by those incremental improvements. Happy to rebase if you still want this, or close it — your call. |
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
2 participants
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
Refactors three internal patterns in the accessor implementation — no behavior change.
Why
Addresses three open issues flagged during the last audit:
require mrocalled inside hot-path function on everynew()#22:require mrowas called inside_all_attributes()on everynew()— now loaded once at module levelrequirewhere a block eval suffices #25: stringeval qq[require $module]replaced with blockeval { require $file }— safer, avoids string eval edge cases_after_*rollback logic copy-pasted in two branches of the accessor #26:_after_*rollback logic was copy-pasted in two branches (class method vs role chain) — unified into a single hook-resolution-then-call patternHow
The hook dispatch loop now resolves the coderef first (
$self->can→ role chain fallback), then calls it once. This eliminates the duplicated rollback block and slightly simplifies the control flow. Same approach applied to the lazy builder resolution.Testing
All 311 existing tests pass. No new tests needed — this is a pure refactor with identical behavior.
Closes #22, closes #25, closes #26.
🤖 Generated with Claude Code
Quality Report
Changes: 1 file changed, 23 insertions(+), 23 deletions(-)
Code scan: clean
Tests: passed (OK)
Branch hygiene: clean
Generated by Kōan post-mission quality pipeline