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

Replace read_rda() by FileIO integration #6

Merged
merged 3 commits into from
Aug 19, 2016
Merged

Conversation

alyst
Copy link
Collaborator

@alyst alyst commented Aug 17, 2016

read_rda() calls is replaced by the generic FileIO load() mechanism as proposed in #2.

  • Do we need any other rdata loading methods?
  • The tests pass, but I'm not sure whether detect_rdata() is actually called. fixed
  • Should the user explicitly type using FileIO or RData should handle this (how?)? FileIO.load is exported

@ararslan
Copy link
Member

Since you have FileIO in REQUIRE and in a using statement, I think you can just prefix the FileIO functions when you overload them and it should just work, but I'm not really sure about that.

@alyst
Copy link
Collaborator Author

alyst commented Aug 17, 2016

@ararslan After briefly looking into FileIO I think that it's not done by overloading. Rather, FileIO explicitly calls Library.load() method, where the Library is a package that provides support for a given file format. But magic- or filename-based dispatching is done by load(filename::String) method declared in FileIO. So upon loading RData one would also need to export FileIO.load.
Is that what @reexport macro is for?

@ararslan
Copy link
Member

ararslan commented Aug 17, 2016

import FileIO: load then include load in the list of exports? I think that should be sufficient.

Using @reexport seems like overkill here since we don't need to expose all of the symbols from FileIO to the user, just load.

@alyst
Copy link
Collaborator Author

alyst commented Aug 17, 2016

@ararslan You're right. Added to PR.

So that should be it: read_rda(...) could be deprecated in favour of load(...) + using RData.

@alyst alyst merged commit 2ff1091 into master Aug 19, 2016
@alyst alyst deleted the ast/fileio_integration branch August 19, 2016 15:12
alyst added a commit that referenced this pull request Aug 23, 2016
Replace read_rda() by FileIO integration
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.

None yet

2 participants