dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.96k stars 4.65k forks source link

System.IO.Compression.ZipFile: CreateEntriesFromDirectory #1546

Open patricksadowski opened 7 years ago

patricksadowski commented 7 years ago

Problem

When creating or updating zip archives there is no option to add a directory with its content. The content of a directory can only be added with custom code by hand. ZipFile.CreateFromDirectory is not an option when building complex zip archives.

Proposed API

namespace System.IO.Compression
{
    public static partial class ZipFile
    {
        public static void CreateFromDirectory(string sourceDirectoryName, string destinationArchiveFileName) { }
        public static void CreateFromDirectory(string sourceDirectoryName, string destinationArchiveFileName, System.IO.Compression.CompressionLevel compressionLevel, bool includeBaseDirectory) { }
        public static void CreateFromDirectory(string sourceDirectoryName, string destinationArchiveFileName, System.IO.Compression.CompressionLevel compressionLevel, bool includeBaseDirectory, System.Text.Encoding entryNameEncoding) { }
        public static void ExtractToDirectory(string sourceArchiveFileName, string destinationDirectoryName) { }
        public static void ExtractToDirectory(string sourceArchiveFileName, string destinationDirectoryName, System.Text.Encoding entryNameEncoding) { }
        public static System.IO.Compression.ZipArchive Open(string archiveFileName, System.IO.Compression.ZipArchiveMode mode) { throw null; }
        public static System.IO.Compression.ZipArchive Open(string archiveFileName, System.IO.Compression.ZipArchiveMode mode, System.Text.Encoding entryNameEncoding) { throw null; }
        public static System.IO.Compression.ZipArchive OpenRead(string archiveFileName) { throw null; }
    }
    [System.ComponentModel.EditorBrowsableAttribute((System.ComponentModel.EditorBrowsableState)(1))]
    public static partial class ZipFileExtensions
    {
        public static System.IO.Compression.ZipArchiveEntry CreateEntryFromFile(this System.IO.Compression.ZipArchive destination, string sourceFileName, string entryName) { throw null; }
        public static System.IO.Compression.ZipArchiveEntry CreateEntryFromFile(this System.IO.Compression.ZipArchive destination, string sourceFileName, string entryName, System.IO.Compression.CompressionLevel compressionLevel) { throw null; }
+       public static System.Collections.Generic.IEnumerable<System.IO.Compression.ZipArchiveEntry> CreateEntriesFromDirectory(this System.IO.Compression.ZipArchive destination, string sourceDirectoryName, string baseEntryName) { throw null; }
+       public static System.Collections.Generic.IEnumerable<System.IO.Compression.ZipArchiveEntry> CreateEntriesFromDirectory(this System.IO.Compression.ZipArchive destination, string sourceDirectoryName, string baseEntryName, System.IO.Compression.CompressionLevel compressionLevel, bool includeBaseDirectory) { throw null; }
        public static void ExtractToDirectory(this System.IO.Compression.ZipArchive source, string destinationDirectoryName) { }
        public static void ExtractToFile(this System.IO.Compression.ZipArchiveEntry source, string destinationFileName) { }
        public static void ExtractToFile(this System.IO.Compression.ZipArchiveEntry source, string destinationFileName, bool overwrite) { }
    }
}
ianhays commented 7 years ago
  • public static System.IO.Compression.ZipArchiveEntry CreateEntryFromDirectory(this System.IO.Compression.ZipArchive destination, string sourceDirectoryName, string entryName) { throw null; }

If you want a directory with all of the items within that directory, you would need to return a list of ZipArchiveEntries. Also, entryName doesn't make much sense when you're adding multiple files unless you want that to be the base path for entries within the directory.

I could see the usefulness of this. The implementation would be easily merge-able with the existing CreateFromDirectory source for a minimal required amount of additional code.

Though there is a simple solution when all files within the directory are at the root e.g.

public static IEnumerable<ZipArchiveEntry> CreateEntriesFromDirectory(this ZipArchive destination, string sourceDirectoryName, CompressionLevel compressionLevel)
{
    foreach (string entry in Directory.EnumerateFileSystemEntries(sourceDirectoryName, "*", SearchOption.TopDirectoryOnly))
        yield return destination.CreateEntryFromFile(entry, Path.GetFileName(entry), compressionLevel);
}

it gets more complex when subfolders are involved as we have to calculate the relative paths for the entry names. Luckily, we already have a function for that.

patricksadowski commented 7 years ago

Yes, the return type could be changed to IEnumerable<ZipArchive>. entryName should be the base path for entries within the directory. I altered my proposal to make the functionality more clear. The API is now based on ZipFile.CreateFromDirectory. I'm not sure if we should add a parameter Encoding entryNameEncoding.

Given a directory structure on local storage

foo/
 |- bar/
     |-info.txt
 |- empty/

I can call the first overload with entryName: "update" to create

/update/bar/info.txt
/update/empty/

The second overload would produce with entryName: "update" and base directory inclusion

/update/foo/bar/info.txt
/update/foo/empty/
ianhays commented 7 years ago

entryName should be the base path for entries within the directory.

I think that makes sense, though entryName isn't a very fitting title anymore. Maybe something like baseEntryPath or baseEntryName?

I'm not sure if we should add a parameter Encoding entryNameEncoding.

My vote would be to leave it out since we don't include it in any of the other extension methods.

The second overload would produce with entryName: "update" and base directory inclusion

The includeBaseDirectory parameter doesn't seem necessary since you could easily add the base directory name to "entryName"/baseEntryName/whatever.

patricksadowski commented 7 years ago

Maybe something like baseEntryPath or baseEntryName?

I changed it to baseEntryName. baseEntryPath doesn't seem to be consistent with other ZipFile and ZipArchive methods.

The includeBaseDirectory parameter doesn't seem necessary since you could easily add the base directory name to "entryName"/baseEntryName/whatever.

Well, I'd like to know why the parameter exists in method ZipFile.CreateFromDirectory. Your explanation leads to the conclusion to also remove the parameter from ZipFile.CreateFromDirectory. Is there a important historical reason to keep the parameter or should we drop it?

ianhays commented 7 years ago

I changed it to baseEntryName. baseEntryPath doesn't seem to be consistent with other ZipFile and ZipArchive methods.

good call.

Well, I'd like to know why the parameter exists in method ZipFile.CreateFromDirectory. Your explanation leads to the conclusion to also remove the parameter from ZipFile.CreateFromDirectory. Is there a important historical reason to keep the parameter or should we drop it?

We've pretty well already made our bed with ZipFile.CreateFromDirectory - if we change it now we break compat. I'm on the fence about including it in CreateEntriesFromDirectory. If you feel that we should to be consistent with CreateFromDirectory then I'm fine with that.

patricksadowski commented 7 years ago

The method should be named CreateEntryFromDirectoryContent to make the includeBaseDirectory parameter obsolete.

It would be nice to have the includeBaseDirectory functionality on board to reduce the amount of custom code for the users. We can add either the includeBaseDirectory parameter or two distinct methods CreateEntryFromDirectory and CreateEntryFromDirectoryContent.

ianhays commented 7 years ago

It would be nice to have the includeBaseDirectory functionality on board to reduce the amount of custom code for the users.

I'm fine with that. Go ahead and include it and we'll see what the API reviewers think once we get it locked down. No need to add a differently-named method.

patricksadowski commented 7 years ago

Is there still API work needed? I would like to implement the proposed API.

ianhays commented 7 years ago

I think the API is good and we're at a reasonable place in the design discussion. The next step before moving forward is to wait for more support. We generally prefer to avoid adding API unless it's something that has wide support from the community or a very strong use-case that provides new functionality. Since this addition so far has neither, it's better to wait until the demand is more clear so that it can be better championed during the API review process.

carlossanlop commented 4 years ago

Triage: Sounds reasonable, we would like to continue with API design and review.

RenderMichael commented 6 months ago

I found myself in need of this API today; is there anything else this proposal needs to be ready for review?