understanding fallback hierarchy / public API

[ssfrr]

I'm working through the load/save methods to figure out how things are dispatching. I'm a little confused about these lines, which AFAICT are so the user can specify the format explicitly as a first argument.

reproduced here for convenience:

# Forced format
function save{sym}(df::Type{DataFormat{sym}}, f::AbstractString, data...; options...)
    libraries = applicable_savers(df)
    checked_import(libraries[1])
    eval(Main, :($save($File($(DataFormat{sym}), $f),
                       $data...; $options...)))
end

function save{sym}(df::Type{DataFormat{sym}}, s::IO, data...; options...)
    libraries = applicable_savers(df)
    checked_import(libraries[1])
    eval(Main, :($save($Stream($(DataFormat{sym}), $s),
                       $data...; $options...)))
end

It seems like the save function that gets called here will be FileIO.save (not the save function from a loader package), which should get caught by the handlers right below, in which case I'm confused as to why it's going to the trouble of looking for a saver, importing the library, and evaling the call in Main (given that those things are happening below anyways).

I tried replacing these save methods with just:

function save{sym}(df::Type{DataFormat{sym}}, f::AbstractString, data...; options...)
    save(File(DataFormat{sym}, f), data...; options...)
end

function save{sym}(df::Type{DataFormat{sym}}, s::IO, data...; options...)
    save(Stream(DataFormat{sym}, s), data...; options...)
end

which still passes all the tests. If I'm understanding this correctly this is also just another way to specify save(File(format"PNG", "myfile.png"), data), which is close to what's documented (for load at least).

So I guess this boils down to 2 questions:

1. Is the public-facing API for forcing the file type:
   • save(format"PNG", "myfile.png", data) or
   • save(File(format"PNG", "myfile.png"), data)
2. do these implementations need all the library-finding and eval magic?

Currently I'm leaning towards "the first" and "no", but there are definitely still things I'm not super clear on and context I don't have.

[timholy]

I wasn't the last to touch those lines (see #94), but I'm guessing the eval is a world-age thing. I suspect that now we could replace it with invokelatest. Whether it passes tests might depend on the order in which you load packages. It's also possible that there's been some julia fixes, so if you really can't trigger any kind of error with the simpler versions, perhaps it would be worth pinging @yuyichao (deliberately backticked to avoid bothering him just yet) to see if he can remember what triggered the change.

I'm also a little uncertain about the purpose of getting libraries in those functions. (@SimonDanisch?)

Finally, I would personally be inclined to go with the save(::File, args...) version rather than the save(::Type{DataFormat{fmt}}, ::AbstractString, args...) version. But since that's opposite of your inclination, we either need to discuss or just keep both.

[timholy]

I believe this is now fixed (#295 if not before).