JuliaIO / JLD.jl

Saving and loading julia variables while preserving native types
MIT License
278 stars 55 forks source link

Add `compress=true/false` option to `@save` macro #318

Closed brenhinkeller closed 1 year ago

brenhinkeller commented 1 year ago

This PR adds an optional first argument to the @save macro, where you can specify either compress=true or compress=false, which will be passed on to the underlying jldopen call.

This PR also adds docstrings for @load and @save, which were previously missing, and adds a few more examples to the README.

brenhinkeller commented 1 year ago

Looks like the nightly failures are unrelated AFAICT

mkitti commented 1 year ago

My objection here is that we may soon move beyond a single compression scheme. Would it make more sense for the value to be :none or :gzip rather than false and true?

brenhinkeller commented 1 year ago

Ah fair! Currently this will just pass whatever option you type after compress= to jldopen, so that should actually already be compatible with this implementation -- would just be a matter of changing the default value false here https://github.com/brenhinkeller/JLD.jl/blob/d6c80f79ab0fbf77df5038c66ceb295991faa9ba/src/JLD.jl#L1214

and in the docs whenever you also change the default value of compress in https://github.com/JuliaIO/JLD.jl/blob/b907a3f37d11e21a776ef7d38da5a4482ee4402c/src/JLD.jl#L162 and https://github.com/JuliaIO/JLD.jl/blob/b907a3f37d11e21a776ef7d38da5a4482ee4402c/src/JLD.jl#L249

brenhinkeller commented 1 year ago

If you prefer we could change that in jldopen now too, though I suppose that would be breaking?

In which case one could also release a non-breaking version with this PR, and then immediately switch to the new syntax and make a breaking release with that -- I'd be happy to make a second PR that switches the syntax

musm commented 1 year ago

In which case one could also release a non-breaking version with this PR, and then immediately switch to the new syntax and make a breaking release with that

Probably much better option. Although why would it be breaking? Can't you have it accept both Bool and Symbol and thus be backwards compatible?

We really don't want to release any breaking changes into this library.

brenhinkeller commented 1 year ago

Oh for sure -- I was just taking @mkitti's comment "Would it make more sense for the value to be :none or :gzip rather than false and true" at face value to mean that there was a plan to stop supporting false and true as options.

If false and true are staying around, then this PR can be merged as-is, and whenever anyone implements symbols as options in jldopen, the @save macro in this PR will already be compatible with that.

musm commented 1 year ago

Gotcha, I don't think we have a plan to stop supporting true and false, but rather to keep them as is and add additional options that are backwards compatible.

brenhinkeller commented 1 year ago

Awesome!

musm commented 1 year ago

I will tag this sans objection @mkitti are you ok with that?

mkitti commented 1 year ago

go ahead