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.

Sign up for GitHub

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

Jump to bottom

Flat Extraction Feature #197

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

Flat Extraction Feature #197

wants to merge 20 commits into from

Conversation

mega5800
Copy link
Contributor

👍🎉 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 added new CLI commands "xfo" and "xfr".
"xfo" allows the user to flat extract MSI files while overwriting files with the same name.
"xfr" allows the user to flat extract MSI files while renaming files with the same name using the suffix "_" with a count.

As discussed, I experienced a few issues with creating test files for this new behaviour.
So I am sending this PR as you suggested.

Please let me know if you have any questions.

Thank you.

@activescott
Copy link
Owner

@mega5800 Thank you for your contribution! I left a few comments that should help you simplify the code and some minor naming convention notes.

To get the test working I think you can just go add new tests to CommandLineExtractTests.cs like this one. Just choose an existing small test file like the NUnit-2.5.2.9222.msi used in several tests there and make sure that the test function itself has a unique name and the GetTestName() function will take care of generating a path where it expects a test file. Then just run that test file without any "expected file" in the src/Lessmsi.Tests/TestFiles/ExpectedOutput/... directory. When you run that new test note these two things:

  1. It will generate a new "actual output" file
  2. The error message when the test fails should show you the path to the generated file.

Then you can copy the generated actual file to the location of the expected file (just be sure to verify that the output in that file is correct). Once you run the test again it should pass.

Let me know if that doesn't help - if it doesn't just try to write the test and add it to the PR and I'll take a look. Sorry I'm not more help right now but I'm on an ARM mac and struggling to get windows to run.

@mega5800
Copy link
Contributor Author

Thank you @activescott for your comments regarding my pull request.

I have addressed most of your comments and marked them as resolved.
Once I finish addressing all of them, I will send an updated pull request for your review.
After you approve the pull request, I will add the tests.
I prefer not to start working on the testing code until all comments are covered.

Thank you.

@mega5800
Copy link
Contributor Author

Hello @activescott.

I covered all your comments.
Please let me know if you have any further comments.

If you are content with the code, I will start working on the tests code.
Thank you.

@mega5800
Copy link
Contributor Author

Hello @activescott.

For some reason, I was unable to produce the actual output files following your steps.
I have added the flat extraction tests to this pull request and as you can see AppVeyor build failed because it doesn’t have the output files.
I would be grateful for your assistance with this issue.

Thank you.

@activescott activescott self-assigned this Jun 25, 2024
@activescott
Copy link
Owner

Thanks @mega5800 ! I'll get looking into this tonight and figure out how to get those files generated. I must have forgotten something as I haven't added new ones in a while myself!

activescott
activescott requested changes Jun 26, 2024
View reviewed changes
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 left a comment with steps that I think will help you get that test working. Tag me in a follow-up comment if it doesn't work! And if it does, request another review from me or tag me and we'll get this merged once those tests are passing.

src/LessMsi.Cli/ExtractCommand.cs
while (++i < args.Count)
filesToExtract.Add(args[i]);

Program.DoExtraction(msiFile, extractDir.TrimEnd('\"'), filesToExtract, getExtractionMode(allArgs[0]));
Copy link
Owner

Choose a reason for hiding this comment

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

is it possible for allArgs.Length < 1 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you please elaborate?

src/Lessmsi.Tests/CommandLineExtractTests.cs Outdated Show resolved Hide resolved
@mega5800
Copy link
Contributor Author

mega5800 commented Jul 3, 2024

Hello @activescott.

I updated the testing logic to adapt for the flat extraction feature, and now I am able to run all the tests successfully.
However, I am facing some difficulties with Codacy errors.
I amended the code according to these comments, however I currently face some medium comments which are not relevant to the code IMO.

First one is about implementing IEquatable interface to a struct I created for grouping file entry comparison bool value and possible error message.
Second one is about throwing exceptions by user code.
The only thing I updated here is the source of the error message, the exception was there.
Would love to get your help here.

Thank you.

@activescott
Copy link
Owner

Great! I'll take a look tomorrow or the next day!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@activescott activescott activescott requested changes

Assignees

@activescott activescott

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@mega5800 @activescott