-
Notifications
You must be signed in to change notification settings - Fork 102
Add write_json to improve compatibility
#381
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #381 +/- ##
==========================================
+ Coverage 90.10% 90.20% +0.10%
==========================================
Files 7 7
Lines 1334 1348 +14
==========================================
+ Hits 1202 1216 +14
Misses 132 132 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/JSON.jl
Outdated
| """ | ||
| function writefile end | ||
|
|
||
| function writefile(::Type{Vector{UInt8}}, x; pretty::Union{Integer,Bool}=false, kw...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not following why we want this or the 2nd definitions? writefile that doesn't write to a file and just returns a byte vector?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to avoid extra conversions between String and Vector{UInt8} for use with Zarr.jl and SmallZarrGroups.jl
I'm also trying to match the style of the writefile function in Parquet2.jl https://expandingman.gitlab.io/Parquet2.jl/#Writing-Data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, looking at this again, writefile isn't a good name for the function, so I changed it to write_json.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a benchmark on the cost of doing the extra String to Vector{UInt8} conversion:
julia> using Chairmarks, JSON, Random
julia> @be randstring(10000) Vector{UInt8}(JSON.json(_)) seconds=2
Benchmark: 52143 samples with 4 evaluations
min 4.807 μs (7 allocs: 19.852 KiB)
median 4.874 μs (7 allocs: 19.852 KiB)
mean 5.566 μs (7 allocs: 19.856 KiB, 0.82% gc time)
max 15.658 ms (7 allocs: 19.883 KiB, 99.92% gc time)
julia> @be randstring(10000) JSON.write_json(Vector{UInt8}, _) seconds=2
Benchmark: 50390 samples with 7 evaluations
min 2.971 μs (4 allocs: 9.914 KiB)
median 3.006 μs (4 allocs: 9.914 KiB)
mean 3.394 μs (4 allocs: 9.919 KiB, 0.82% gc time)
max 8.692 ms (4 allocs: 9.945 KiB, 99.92% gc time)writefile to improve compatibilitywrite_json to improve compatibility
This PR adds a
JSON.write_jsonfunction with the same functionality and keyword arguments as the newJSON.jsonfunction. This function is provided for backward compatibility when upgrading fromolder versions of JSON.jl where the
jsonfunction signature differed.The idea is that
JSON.write_jsonwill be backported to v0.21.5EDIT: Changed the name from
writefiletowrite_json