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

AddAllFromDirectoryContentOnly extension method for IWritableArchive #450

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

iron9light
Copy link

Only write content (will ignore timestamp). This method is used to make sure same contents of a directory will always output the same binary.

…gnore timestamp). This method is used to make sure same contents of a directory will always output the same binary.
@adamhathcock
Copy link
Owner

So you basically just want an overload to ignore timestamps? Can they just be an option instead of a new method?

@adamhathcock adamhathcock self-requested a review May 22, 2019 12:46
@iron9light
Copy link
Author

iron9light commented May 22, 2019

Indeed, it will do two things.

  1. Make sure the order of adding files are the same for the same folder.
  2. Make sure only content, but no other info, will add to the archive.

if I only add a default parameter bool contentOnly = false, it will not let the user know the 1st thing will be done.
I use a different method name here. Because when I use your lib, everything goes fine until I'm aware that folders with same content may generate a different binary.
(My use case: I will archive a folder and upload to somewhere with checksum, when downloading, will check if the dest folder does exist. if so, I will archive the local folder again and compare the checksum with the remote checksum file)
I think most of the user will not be aware of the output difference even the content are the same.
Maybe we should use a better name for this case. (AddAllFromDirectoryIdempotent ?)
Any suggestions?

B.T.W. I fixed a bug. Correct me if I'm wrong.

@adamhathcock
Copy link
Owner

I still think adding one or more parameters to the options class is the correct thing to do. I like your use case so I have no issues with the functionality.

Yeah I saw the bug fix. Just shows no one uses NET35

@iron9light
Copy link
Author

Sorry, I misunderstood what's you mean. I just found it right after I updated this PR. I thought you want me to add one more parameter for those extension methods.
I will open another PR for that.

@iron9light
Copy link
Author

If add a ContentOnly property of Options, what will the writer do when the user passes a modified timestamp? Just ignore it?
So I think a better design if you wanna content only the first class feature:

public interface IContentOnlyWritableArchive: IArchive
{
    void RemoveEntry(IArchiveEntry entry);

    IArchiveEntry AddEntry(string key, Stream source, bool closeStream, long size = 0);

    void SaveTo(Stream stream, WriterOptions options);
}

public interface IWritableArchive: IContentOnlyWritableArchive
{
    IArchiveEntry AddEntry(string key, Stream source, bool closeStream, long size, DateTime modified);
}

It looks like a breaking change.

If we ignore the interface change. When and where the paths will be sorted?

So I think extension methods is best choice for now.

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