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

Generalized factories to readers and writers. #709

Merged
merged 6 commits into from
Dec 6, 2022
Merged

Generalized factories to readers and writers. #709

merged 6 commits into from
Dec 6, 2022

Conversation

vpenades
Copy link
Contributor

What's changed:

  • Created a new entry point for registering factories in a new folder called Factories.Factory (for lack of a better name)
  • I've moved the ArchiveFactories (ZipArchiveFactory, etc) to the new Factories folder
  • Added new interfaces IReaderFactory and IWriterFactory

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 and IWriterFactory 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.

@vpenades
Copy link
Contributor Author

vpenades commented Dec 5, 2022

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.

Copy link
Owner

@adamhathcock adamhathcock left a 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";
Copy link
Owner

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.

Copy link
Contributor Author

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.

Copy link
Owner

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

Copy link
Contributor Author

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?

@adamhathcock adamhathcock merged commit fc02d32 into adamhathcock:master Dec 6, 2022
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.

2 participants