Adopt #58: flatten the map element loop - #78
Merged
Conversation
Brings gitsang's commit in as their own, so the authorship lands in history. Their defaults_test.go hunk is dropped -- that file no longer exists -- and the equivalent pin follows in the next commit. The README restructure and example/output.json are dropped too: the README's usage block is kept byte-identical to example/main.go, and a checked-in output would be stale the moment it was written, since the example's SetDefaults fills a field from math/rand. What this changes is less than #58 reports. Its premise does not reproduce: the snippet in the PR body already prints {"Maps":{"hello":{"Name":"hello"}}} on abebf4b, the commit it branched from, and its own test passes there unmodified. Every test in the suite passes unchanged after the port, which is the evidence that nothing observable moved. What is left is still worth having. The old loop switched on the element kind twice, once behind a pointer and once not, spelling out struct/slice/map in both. This switches once: dereference first, then copy only when the value is not addressable -- which a dereferenced pointer already is, so the copy and the write-back now happen exactly where they are needed. The nil element becomes explicit as well. It used to be skipped because Elem() of a nil pointer has kind Invalid and so matched no case, which is the right outcome reached by accident. The outcome is unchanged, and the next commit pins it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JjAoN3BBvc6dfT9pog3kMB
The flattened loop makes the nil element explicit, so the coverage gate asks for it, and the case is worth a pin of its own regardless: the key stays and nothing is allocated behind it. That is not obvious sitting next to a nil pointer field, where a tag does allocate -- a map element is written through rather than copied back, so allocating one would have to be a deliberate choice, and it is not made. Both spellings are covered, the caller's nil and a tag whose JSON says null. Neither is a behavior change: master already left both alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JjAoN3BBvc6dfT9pog3kMB
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #78 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 188 191 +3
=========================================
+ Hits 188 191 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
#76 landed while this branch was open, and it groups setField's per-field context into a fieldTag. The flattened element loop keeps its shape; only the recursion call changes, from setField(v, "", false) to setField(v, fieldTag{fieldName: tag.fieldName}), so an error raised inside a map element still names the field it came from. Nothing else moved: the loop's structure, the nil guard and the write-back are as merged, and the suite passes with the gate at 100%. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JjAoN3BBvc6dfT9pog3kMB
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
3 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.
Adapts #58 (@gitsang) onto the new test suite. Their commit is merged rather than
cherry-picked, so the authorship lands in history.
This changes no behavior, which is not what #58 claims. Its premise does not
reproduce: the snippet in the PR body already prints
{"Maps":{"hello":{"Name":"hello"}}}onabebf4b, the commit it branched from,and the test it ships passes there unmodified. Every test in the current suite also
passes unchanged after the port — that is the evidence nothing observable moved.
What is left is still worth taking:
and once not, spelling out
struct/slice/mapin both. It now switches once:dereference first, then copy only when the value is not addressable — which a
dereferenced pointer already is. The copy and the write-back now happen exactly
where they are needed, rather than in a duplicated arm.
Elem()of a nil pointer has kind
Invalidand so matched no case: the right outcome,reached by accident. The outcome is unchanged and now pinned, both for the
caller's
niland for a tag whose JSON saysnull.Dropped from #58: the README restructure (it reinstates the CircleCI badge, and
the usage block is kept byte-identical to
example/main.go) andexample/output.json(the example fills a field frommath/rand, so a checked-inoutput is stale the moment it is written).
Verified:
make lint,make test,make coverat 100%, and the suite on thedeclared floor (
GOTOOLCHAIN=go1.21.13).🤖 Generated with Claude Code
https://claude.ai/code/session_01JjAoN3BBvc6dfT9pog3kMB
Generated by Claude Code