weichsel / ZIPFoundation

Effortless ZIP Handling in Swift
MIT License
2.34k stars 263 forks source link

fix crash if fopen returns nil #234

Closed LGriffioen closed 2 years ago

LGriffioen commented 3 years ago

Fixes crash that can happen if you try to save an archive to Google Drive in streaming mode

Changes proposed in this PR

Tests performed

Further info for the reviewer

Open Issues

weichsel commented 2 years ago

Changes look good - Thanks for submitting this PR.

We just have to fix the 2 minor CI errors:

Let me know if I can help with anything. You could also grant me push rights to your fork and I can look into fixing those CI issues.

LGriffioen commented 2 years ago

thanks for taking a look! i fixed up the swiftlint issue and it appears to have masked the code coverage issue. i did look into writing a unit test for the case where fopen returns nil, but couldn't figure out how to trigger it without refactoring the code to support passing in mocks.

i talked to my org and they're not thrilled about granting access to our private forks, but they did suggest merging into a non-development branch in this repo so that you could add tests to it. does that sound like a reasonable way forwards? if not, i can push back on our folks and see if we can come up with something that works better for you.

weichsel commented 2 years ago

Hi Luke, Thanks for the quick turnaround.

i fixed up the swiftlint issue and it appears to have masked the code coverage issue. i did look into writing a unit test for the case where fopen returns nil, but couldn't figure out how to trigger it without refactoring the code to support passing in mocks.

The coverage check is strictly line-based. Since the throw is now on the same line, the line is considered "covered" by Xcode. I think this is OK for now.

Your addition of checking the return value of fopen is a clear improvement - so I think we can merge this. Thanks for your PR!