support both String and Symbol keys for all dict-operations on Objects#421
Conversation
…ey::Symbol and vice versa plus respective tests
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #421 +/- ##
==========================================
+ Coverage 90.05% 90.23% +0.18%
==========================================
Files 7 7
Lines 1347 1352 +5
==========================================
+ Hits 1213 1220 +7
+ Misses 134 132 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Background: julia> obj = JSON.parse("""{"a": "a"}""");
julia> haskey(obj, :a)
true
julia> obj[:a] = "b"
ERROR: MethodError: no method matching setindex!(::JSON.Object{String, Any}, ::String, ::Symbol)
The function `setindex!` exists, but no method is defined for this combination of argument types.
Closest candidates are:
setindex!(::AbstractDict, ::Any, ::Any, ::Any, ::Any...)
@ Base abstractdict.jl:560
setindex!(::JSON.Object{K, V}, ::Any, ::K) where {K, V}
@ JSON ~/.julia/dev/JSON/src/object.jl:160This PR solves that issue. |
|
BTW, @quinnj, are you planning a new release? |
quinnj
left a comment
There was a problem hiding this comment.
Thanks for the PR! This is a good follow-up to #420 for consistency.
Review Summary
Implementation: Clean and follows the existing pattern - simple delegation with key conversion. No issues here.
Tests: Comprehensive coverage for the methods added. The tests properly check both directions (Object{String} with Symbol keys and Object{Symbol} with String keys).
One Gap Remaining
After this PR, there's still one set of methods missing for full consistency - the get methods:
julia> obj = JSON.parse("""{"a": 1}""")
julia> get(obj, :a, 0) # fails
ERROR: MethodError: no method matching find_node_by_key(::JSON.Object{String, Any}, ::Symbol)
julia> get(() -> 0, obj, :a) # also fails
ERROR: MethodError: ...To complete the String/Symbol interoperability, we'd also need:
Base.get(f::Base.Callable, obj::Object{String}, key::Symbol) = get(f, obj, String(key))
Base.get(f::Base.Callable, obj::Object{Symbol}, key::String) = get(f, obj, Symbol(key))
Base.get(obj::Object{String}, key::Symbol, default) = get(obj, String(key), default)
Base.get(obj::Object{Symbol}, key::String, default) = get(obj, Symbol(key), default)Suggestion: Either add these to this PR, or we can merge this as-is and follow up with another PR. The current changes are valuable on their own.
Summary Table (after this PR)
| Method | Object{String} + Symbol |
Object{Symbol} + String |
|---|---|---|
getindex |
✅ #420 | ✅ #420 |
setindex! |
✅ this PR | ✅ this PR |
delete! |
✅ this PR | ✅ this PR |
haskey |
✅ existing | ✅ this PR |
get(callable, ...) |
❌ missing | ❌ missing |
get(..., default) |
❌ missing | ❌ missing |
Overall this looks good to merge as-is, with an optional follow-up for the get methods.
quinnj
left a comment
There was a problem hiding this comment.
Looks good! Merging now. I'll follow up with the missing get methods in a separate commit.
Completes String/Symbol interoperability for Object by adding get
methods that accept cross-key-types:
- get(f::Callable, obj::Object{String}, key::Symbol)
- get(f::Callable, obj::Object{Symbol}, key::String)
- get(obj::Object{String}, key::Symbol, default)
- get(obj::Object{Symbol}, key::String, default)
This follows up on #421 which added setindex!, delete!, and haskey
for cross-key-types.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@quinnj May I ask when you plan the next release? |
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
setindex!(),delete!()forobj::Object{String},key::Symboland vice versahaskey(obj::Object{String}, key::Symbol)