Skip to content

Make extractData: and extractDataFromFile: compatible with Swift 2 throws#29

Merged
abbeycode merged 6 commits intoabbeycode:masterfrom
amosavian:master
Feb 12, 2016
Merged

Make extractData: and extractDataFromFile: compatible with Swift 2 throws#29
abbeycode merged 6 commits intoabbeycode:masterfrom
amosavian:master

Conversation

@amosavian
Copy link
Copy Markdown
Contributor

adding nullable to return values of these functions made them more swifty

Comment thread Source/UZKArchive.h Outdated
- (NSData *)extractData:(UZKFileInfo *)fileInfo
- (nullable NSData *)extractData:(UZKFileInfo *)fileInfo
progress:(nullable void (^)(CGFloat percentDecompressed))progress
error:(NSError **)error;
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Please fix the indentation of the lines below to still align on the :.

@abbeycode
Copy link
Copy Markdown
Owner

Thanks for this! Could you add a couple of Swift unit tests verifying that they throw correctly? Add a test case class ExtractDataTests_Swift.swift and copy over testExtractData_NoPassword and testExtractData_InvalidArchive from ExtractDataTests.m, rewriting in Swift and asserting about the caught errors. Please be sure to add the new test to the OS X and iOS test targets.

(And check out the minor formatting nitpicks I made inline).

I'll copy over the remaining tests later on.

@amosavian
Copy link
Copy Markdown
Contributor Author

I've updated the indentation issue but for test we should have a test file with correct header and corrupted data. I can't make such file.
Your previous code would crash app in swift but this code can handle this with newly introduced try,catch block:
let thumbData = archive.extractDataFromFile("docProps/thumbnail.jpeg", progress: nil, error: error)
will become:
let thumbData = try? archive.extractDataFromFile("docProps/thumbnail.jpeg", progress: nil)
or in case you want handle error:

    do {
        let thumbData = try archive.extractDataFromFile("docProps/thumbnail.jpeg", progress: nil)
    } catch let error as NSError {
        NSLog("Error load thumbnail data: \(error.localizedDescription)");
    } catch {}

@abbeycode
Copy link
Copy Markdown
Owner

If you take the existing tests as a starting point, you need to add do/catch around the UnzipKit call, and set a Boolean variable to true in the catch, asserting that it's true in the body of the test. Does that make sense? I can modify your code above to show you what I mean if it wasn't clear.

@amosavian
Copy link
Copy Markdown
Contributor Author

Is it okay?

let data = try archive.extractDataFromFile("Test File A.txt", progress: { (percentDecompressed) -> Void in
NSLog("Extracting, \(percentDecompressed) complete");
})
} catch let error as NSError {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

(This applies to both tests)

This is pretty close, but I think it would be better to declare a var extractError: ErrorType! outside of the do/catch and then assign it in the catch block. You can then remove the general catch, and assert that extractError (a) is not nil, (b) is an NSError, and (c) has the correct error code (as you were already asserting.

The problem with having your assertions inside the catch block is that that if there were no error thrown, or if it weren't an NSError, the test would still pass (since the assertions would be skipped over).

This is looking good though, thanks for sticking with it!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's your code but imo it's better to handle thing in a swifty way. That approach may be good for objc code but makes no sense in swift code, may result in confusion for a swift programmer.
I copied your own code into swift, though I think it's better to assess different things in swift as it much more error proof in handling nil values than objc

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I don't think that what I suggested is any less "Swifty". This isn't production Swift code, but rather a test to validate output. If you prefer, declare the variable as var extractError: ErrorType? instead, but my point was that if the code doesn't correctly write the error, or writes a type other than expected, the tests I suggested will fail. As you wrote them, they would still succeed.

You can try this out - if you update extractDataFromFile to return nil on its first line, and run your tests, I believe they will still succeed, as no error will be thrown.

The best way to handle these tests would be to use a better matching framework than XCTAssert. Someday I'll switch the tests to use something like Nimble, which would allow you to write something like:

expect{ try archive.extractDataFromFile("Test File A.txt") }.to(throwError {err in
    expect(err.code) == UZKErrorCode.InvalidPassword
})

I'd rather not start using Nimble quite yet, though.

@abbeycode abbeycode merged commit 06337cb into abbeycode:master Feb 12, 2016
abbeycode added a commit that referenced this pull request Feb 12, 2016
@abbeycode abbeycode added this to the 1.7 milestone Feb 12, 2016
@abbeycode abbeycode self-assigned this Feb 12, 2016
@abbeycode
Copy link
Copy Markdown
Owner

v1.7 is live on CocoaPods now, including this fix. Thanks @amosavian for the work you put in!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants