activescott / lessmsi

A tool to view and extract the contents of an Windows Installer (.msi) file.
https://lessmsi.activescott.com
MIT License
1.29k stars 150 forks source link

Flat Extraction Feature #197

Closed mega5800 closed 1 month ago

mega5800 commented 2 months ago

:+1::tada: First off, thanks for taking the time to contribute! :tada::+1:

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 commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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!

mega5800 commented 2 months ago

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 commented 2 months ago

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

mega5800 commented 2 months ago

@activescott any updates from your side? Thank you.

activescott commented 2 months ago

Sorry no update yet. I didn't have time this weekend. I'll look at it as soon as I can. Hopefully this week

activescott commented 2 months ago

@mega5800 Check out 7e7de8dd4fa3f9ed827f9d905c08e202b7cb0070 and eec6c43e744521eda3740f3fae60fb47598b1e13 and let me know if you have questions or concerns. A couple of notes from my memory of what I saw:

Please do give it a good review as it is late for me and I was hacking my way through a bit quickly :) I also didn't review your latest changes thoroughly. Why don't you look at this and make sure you feel good about it and if you do I'll take one last look before we merge.

mega5800 commented 2 months ago

Hello @activescott.

I reviewed your changes in the CompareEntriesResult file. Additionally, I updated the code to pass Codacy tests. As a result, the pull request now successfully passes both our internal tests and Codacy tests.

I think we are ready for merge, unless you have any other comments.

Thank you.

mega5800 commented 2 months ago

@activescott Just updated the help text

activescott commented 1 month ago

:tada: This PR is included in version 2.1.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: