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

Rewrite from scratch #15

Merged
merged 4 commits into from
Aug 3, 2015
Merged

Rewrite from scratch #15

merged 4 commits into from
Aug 3, 2015

Conversation

timholy
Copy link
Member

@timholy timholy commented Aug 2, 2015

This adds magic number support, and tries to address real-world
complications such as:

  • The same extension might mean two or more different formats
  • The extension might lie about the format
  • Some filesformats don't have magic numbers
  • Some magic numbers are found in weird places in files

I'll add some documentation for this, but I may be done for the day.

This adds magic number support, and tries to address real-world
complications such as:
- The same extension might mean two or more different formats
- The extension might lie about the format
- Some files don't have magic numbers
- Some magic numbers are found in weird places in files
@timholy
Copy link
Member Author

timholy commented Aug 2, 2015

OK, I think this is in good shape now. Probably the most thoroughly-documented module I've ever created, too.

@timholy
Copy link
Member Author

timholy commented Aug 2, 2015

Note that reviewing this might be easiest if you first work through the README, which you can do most easily from the front page and selecting this branch.

end


abstract Formatted{F<:DataFormat} # A specific file or stream
Copy link
Member

Choose a reason for hiding this comment

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

Would AbstractFormat work here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, and that's what I had originally. But somehow AbstractFormat sounds to me more like a format specification, whereas these are supposed to represent instances (e.g., particular files).

- register packages that should be loaded before attempting to work
with a particular file format.
- add utilities to retrieve or skip over the magic bytes
- write a lot of help!
@ssfrr
Copy link
Contributor

ssfrr commented Aug 3, 2015

This is fantastic, I really like the API, it feels very right.

I'm looking forward to packages not needing their own MyPkg.open or open_myformat methods.

@timholy
Copy link
Member Author

timholy commented Aug 3, 2015

Glad you like it, @ssfrr.

I'm looking forward to packages not needing their own MyPkg.open or open_myformat methods.

There may still be some of that; for example, are we really going to try to register each of the ~80 file formats supported by ImageMagick? As an alternative, Images might still retain an imread that says "use ImageMagick if nothing more specific is suggested."

@ssfrr
Copy link
Contributor

ssfrr commented Aug 3, 2015

OK, maybe I just meant I'm looking forward to AudioIO not needing an AudioIO.open. 😸

I'll probably end up registering whatever libsndfile supports, and using Wav.jl for .wav files. I'll have to think a bit about what options will make sense.

@SimonDanisch
Copy link
Member

Looks great!!! I can't express how glad I am that you simply tackled this ;)
It seems to cover all my use cases, so I'll just start using it and see how it goes.

@timholy
Copy link
Member Author

timholy commented Aug 3, 2015

Great. I'll merge this, and start playing with it myself. I am sure there will need to be little tweaks made here and there as we try it out in packages.

timholy added a commit that referenced this pull request Aug 3, 2015
@timholy timholy merged commit eeb94ff into master Aug 3, 2015
@timholy timholy deleted the teh/refactor branch August 3, 2015 11:43
SimonDanisch added a commit to JuliaIO/MeshIO.jl that referenced this pull request Aug 9, 2015
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.

4 participants