Skip to content
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

Add some methods to make JLSOFile more Dict-like #108

Merged
merged 5 commits into from
Jun 2, 2021

Conversation

nickrobinson251
Copy link
Contributor

  • adds keys, haskey, get and get! for JLSOFile (but not iterate as i didn't have a use-cae for it); closes Should JLSOFile have get, get!, keys and haskey methods #107
  • fixes JLSOFile() to return a JLSOFile, whereas on master it errrors:
    julia> JLSOFile()
    ERROR: MethodError: no method matching JLSOFile(::Dict{Union{}, Union{}}; version=v"4.0.0", julia=v"1.6.0", format=:julia_serialize, compression=:gzip, image="")
    Closest candidates are:
    JLSOFile(::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any, ::Any) at /Users/nick/repos/JLSO.jl/src/JLSOFile.jl:2 got unsupported keyword arguments "version", "julia", "format", "compression", "image"
    JLSOFile(; version, julia, format, compression, image, kwargs...) at /Users/nick/repos/JLSO.jl/src/JLSOFile.jl:70
    JLSOFile(::Pair{Symbol, B} where B...; kwargs...) at /Users/nick/repos/JLSO.jl/src/JLSOFile.jl:89
    ...
    Stacktrace:
    [1] JLSOFile(; version::VersionNumber, julia::VersionNumber, format::Symbol, compression::Symbol, image::String, kwargs::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
     @ JLSO ~/repos/JLSO.jl/src/JLSOFile.jl:78
    [2] JLSOFile()
     @ JLSO ~/repos/JLSO.jl/src/JLSOFile.jl:78
    [3] top-level scope
     @ REPL[2]:1
  • adds a save(io, ::JLSOFile) method

Together, this allows users to write code like

jlso = isfile(filename) ? read(filename, JLSOFile) : JLSOFile()
get!(jlso, :qux) do
    make_qux()
end
JLSO.save(filename, jlso)

or

if isfile(filename) 
    jlso = read(filename, JLSOFile)
    haskey(jlso, :qux) && return jlso[:qux]
else
    jlso = JLSOFile()    
end
jlso[:qux] = make_qux()
JLSO.save(filename, jlso)

The main advantage is this allows working with a JLSOFile similar to working with the Dict that would be returned by JLSO.load(filename), but with JLSOFile lazily deserialising (unlike load)

return JLSOFile(
Dict(kwargs);
data;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't love this magic keyword constructor, which has some "special" keywords (version, julia, ...) and then eats all the others as key/value pairs to be serialised... especially as we have a :key => value pair constructor already: JLSOFile(data::Pair{Symbol}...; kwargs...) = JLSOFile(Dict(data); kwargs...)

But I think removing this is another issue (and breaking). Maybe we should open an issue about that.

But whether we have magic-keywords or not, i think this is now the correct behjaviour for JLSOFile() (before it was a cryptic error)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, :+1 for just using pairs for data objects. I think this was proposed cause some folks like creating dataframes with the keyword constructor and we often don't change the special keywords. Please open an issue to delete this method as it should be easy enough to deprecate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/file_io.jl Show resolved Hide resolved
test/JLSOFile.jl Show resolved Hide resolved
@oxinabox oxinabox removed their request for review June 2, 2021 16:23
@codecov
Copy link

codecov bot commented Jun 2, 2021

Codecov Report

Merging #108 (0a08047) into master (dbf7be6) will increase coverage by 0.11%.
The diff coverage is 100.00%.

❗ Current head 0a08047 differs from pull request most recent head 7c75b90. Consider uploading reports for the commit 7c75b90 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #108      +/-   ##
==========================================
+ Coverage   97.56%   97.67%   +0.11%     
==========================================
  Files           6        6              
  Lines         205      215      +10     
==========================================
+ Hits          200      210      +10     
  Misses          5        5              
Impacted Files Coverage Δ
src/JLSOFile.jl 93.33% <100.00%> (+2.85%) ⬆️
src/file_io.jl 94.44% <100.00%> (+0.10%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dbf7be6...7c75b90. Read the comment docs.

Copy link
Member

@rofinn rofinn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, would you mind bumping the minor release?

return JLSOFile(
Dict(kwargs);
data;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, :+1 for just using pairs for data objects. I think this was proposed cause some folks like creating dataframes with the keyword constructor and we often don't change the special keywords. Please open an issue to delete this method as it should be easy enough to deprecate.

src/file_io.jl Show resolved Hide resolved
test/JLSOFile.jl Show resolved Hide resolved
@nickrobinson251 nickrobinson251 merged commit aa6c1f7 into master Jun 2, 2021
@nickrobinson251 nickrobinson251 deleted the npr/jlsofile-keys-get branch September 28, 2022 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should JLSOFile have get, get!, keys and haskey methods
2 participants