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

Overwrite Extraction with Original Folder Structure #212

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

Conversation

mega5800
Copy link
Contributor

@mega5800 mega5800 commented Dec 12, 2024

👍🎉 First off, thanks for taking the time to contribute! 🎉👍

Please fill out the following checklist:

If you need any help at all, feel free to submit the PR and @-mention activescott and I'll be happy to assist!

Hello @activescott.

I took care of this ticket.

I implemented a logic for detecting and deleting any duplicate files after a file extraction.
I added some tests to cover this feature from different points of view, such as regular extraction, extraction with a defined path and an extraction with specific files.

The newly added tests use a new MSI file I added called “AppleMobileDeviceSupport64.msi”.
I know this file contains duplicate files, thus making it a prefect match for testing this new overwrite extraction.

I’d greatly appreciate if you could review my pull request.
Thank you.

Copy link
Owner

@activescott activescott left a comment

Choose a reason for hiding this comment

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

@mega5800 Great work here. I love the tests! I added a comment to one line that I think we should consider. Let me know what you think.

src/LessMsi.Core/Msi/Wixtracts.cs Outdated Show resolved Hide resolved
@mega5800 mega5800 requested a review from activescott December 21, 2024 21:37
Copy link
Owner

@activescott activescott left a comment

Choose a reason for hiding this comment

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

I requested one change to simplify the ExtractCommand and make it easier to read. I sent you a PR for the change. Once that is done, I will approve. Thanks!

Comment on lines +42 to +81
if (isFlatExtractionRequired(commandArgument))
{
extractionMode = ExtractionMode.OverwriteFlatExtraction;
if (commandArgument[commandArgument.Length - 1] == 'o')
{
extractionMode = ExtractionMode.OverwriteFlatExtraction;
}
else if (commandArgument[commandArgument.Length - 1] == 'r')
{
extractionMode = ExtractionMode.RenameFlatExtraction;
}
}
else if (commandArgument[commandArgument.Length - 1] == 'r')
else
{
extractionMode = ExtractionMode.RenameFlatExtraction;
if (isRegularExtracionWithOverwriteRequired(commandArgument))
{
extractionMode = ExtractionMode.OverwriteExtraction;
}
}

return extractionMode;
}

private bool isFlatExtractionRequired(string commandArgument)
{
bool flatExtractionRequiredFlag = false;

if (commandArgument.Length > 1)
{
flatExtractionRequiredFlag = commandArgument[1] == 'f';
}

return flatExtractionRequiredFlag;
}

private bool isRegularExtracionWithOverwriteRequired(string commandArgument)
{
bool regularExtracionWithOverwriteFlag = commandArgument[commandArgument.Length - 1] == 'o';

return regularExtracionWithOverwriteFlag;
}
Copy link
Owner

Choose a reason for hiding this comment

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

Please see mega5800#1 for a simplification here.

/// </summary>
None,
Default,
Copy link
Owner

Choose a reason for hiding this comment

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

NOTE: Changing this would be a breaking change to anyone using the library. I don't think that's a problem since this is the LessMsi.Cli library and to my knowledge only people depend on the Lessmsi.Core library. I am okay making this change, just want to point it out.

@activescott
Copy link
Owner

also, please update the branch (you can use the github ui to update it and then pull it again locally from your own branch).

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