GH-71: Centralise the record-or-throw policy that is actually shared - #117
Merged
Conversation
The issue lists six identical sites. That was true when it was filed; #65 made three of them differ, and the differences are load-bearing rather than drift. So this centralises the three that genuinely share the policy and marks the two that do not, with the reason at the call site: - handleMappingException() records BEFORE raising, because it IS the catch site. Routing it through the helper would leave an aborting run with no record at all. - the collection element loop records inside the element's own path segment, so the record names the element rather than the collection. The helper records at whatever path the context currently carries. Converting all six, as the issue proposes, would have broken both - which is why the helper's own docblock names them: the next reader should not be able to 'finish' the refactoring without meeting the reason not to. MappingContext::recordOrThrow() raises before recording, for the reason the three sites already documented separately: when the run aborts, the exception reaches a catch that records it, so recording here too files the same failure twice. Behaviour-neutral as the issue requires - the whole suite was green before the helper's own tests were added. Verified by mutation: swapping the order inside the helper fails both the new unit test and the integration test that pins the duplicate-record contract from #65.
…s refutable
Review measured my justification for one exemption and found it false. I
claimed the collection element loop could not use the helper because the
path would be lost; withPathSegment() restores the path in a finally, so a
throw from inside the closure propagates cleanly and the record would land
at the element path just fine.
The real blocker is the ORDER: that site must record even while aborting,
and the helper raises first. 'Finishing' the refactoring loses the
element's record silently, since the caller still gets its exception:
current records=2 paths=["$.values.0", "$.values"]
refactored records=1 paths=["$.values"]
That matters because the comment's only job is to stop a future reader
from making exactly that change. A guard comment naming a defeatable
reason is worse than none - the reader checks it, finds it false, concludes
the exemption is stale, and proceeds. All three copies now state the
ordering.
Renamed to throwOrRecord() on the simplifier's suggestion, and it earns
its keep: both exemptions turn on the order, so a name that hides it makes
them look arbitrary and needs prose to compensate. With the order in the
name, 'this site records BEFORE raising' explains itself against a helper
that says it does the opposite, and the docblock loses its bullet list -
which also removed a dependency pointing the wrong way, a low-level
context object describing two callers' internals.
Also two cross-references left stale by this branch's own extraction: both
described sites as 'consulting shouldAbortOnError()' when they now call
the helper.
The guard comment said an aborting run would carry no record of the
element 'at all'. Measured on a nested payload, a deeper record written
further inside the element survives:
current ["$.values.0.number", "$.values.0", "$.values"]
refactored ["$.values.0.number", "$.values"]
So the loss is the element's OWN record, and the surviving deeper one is
what makes the report look complete. An overstated claim in a guard
comment is the same failure as a refutable one: a reader who checks it on
the nested case finds records present and concludes the warning is wrong.
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
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
1 participant
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 #71.
The issue asks to centralise a record-or-throw policy it says is duplicated at six identical sites, as a behaviour-neutral refactoring. That was true when it was filed. #65 changed three of them, and the differences turned out to be load-bearing — so this converts the three that genuinely share the policy and exempts two, each marked with the reason at its own call site.
Why not all six
Converting the two exempted sites loses a record, silently, because the caller still gets its exception. Measured:
handleMappingException()records=1 ["$.number"]records=3 [...]records=0 []records=2— the element's own record gonehandleMappingException()records before raising because it is the catch site the helper's throw would reach. The element loop records before raising too, from inside the element's path segment so the record names the element.What review caught
My justification for one exemption was refutable. I wrote that the element loop could not use the helper because the path would be lost. The reviewer measured it:
withPathSegment()restores the path in afinally, so a throw propagates cleanly and the record would land correctly. The real blocker is the order.That distinction matters more than it looks. A guard comment's only job is to stop a future reader from making the change — and a reader who checks a defeatable reason, finds it false, concludes the exemption is stale and proceeds. Same for an overstated one: the comment first said an aborting run would carry "no record of the element at all", but on a nested payload a deeper record survives and makes the report look complete.
The rename does real work.
recordOrThrow()→throwOrRecord(), suggested by the simplicity review. Both exemptions turn on the order, so a name that hides it makes them look arbitrary and needs prose to compensate. With the order in the name, "this site records BEFORE raising" explains itself against a helper that says it does the opposite — and the docblock lost its bullet list, which also removed a dependency pointing the wrong way (a low-level context object describing two callers' internals).Plus two cross-references this branch's own extraction left stale.
Verification
Behaviour-neutral as the issue requires:
mainand this branch produce byte-identical output across abort and lenient lanes, property mismatch, root failure, collection element andmapWithReport.composer ci:testgreen, verified by exit code: 373 tests, 1211 assertions; PHPStan max, Rector, CGL and CPD clean.Mutation-checked: swapping the order inside the helper fails both its own unit test and the integration test pinning the duplicate-record contract from #65.