-
Notifications
You must be signed in to change notification settings - Fork 480
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
Generalized factories to readers and writers. #709
Conversation
I think I've covered all the functions that depended on explicit case per case formats, and now everything is generalized into the factories. On my side I think this PR is ready for review. Let me know if there's something you don't like and want it changed/removed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor question but looks good!
public override IEnumerable<string> GetSupportedExtensions() | ||
{ | ||
yield return "tar"; | ||
yield return "tgz"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more extensions for LZip/BZip2 types? tbz or tlz? those are guesses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just added the extensions reported by wikipedia, didn't know tar had so many flavours!
I was tempted to add the "tar.*" variants, but I am aware some APIs don't handle well extensions with dots and can produce misleading results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nah, we'll let the user handle nested extensions...
some of these we don't handle though... LZMA we could but XZ, zstd we don't *yet and not sure what compress is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've commented these extensions, I take it tZ and taZ are supported?
What's changed:
With these new interfaces in place, I've been able to clean up some code; the idea is to eventually get rid of all the
if zip else if gzip else if rar else if tar...
spaghetti blocks. This will simplify adding new formats because everything will be centralized in a single factory class.Supporting the TAR.GZ and variants has been specially tricky, I wanted to retain as much of the original logic as possible to avoid breaking something, so I had to create an internal overridable method
Factory.TryOpenReader
that handles the internal RewindableStream logic. Fortunately, new formats will not needs this.From now on, adding a new archive format is about extending "Factory" class, and implement any of
IArchiveFactory
,IReaderFactory
andIWriterFactory
on it.SevenZipFactory is an example of a factory implementing only IArchiveFactory but not IReaderFactory. To some degree, these interfaces work as decorators, so an archive format only needs to implement the interfaces it can support.
Future: there's still some spaghettis around, like getting multipart files, or creating a writeable archive... I think these could be done by adding additional decoration interfaces.