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

Added decoder for URL. Also, added tests for dates and URLs. #127

Merged
merged 3 commits into from
Nov 6, 2016

Conversation

codeOfRobin
Copy link
Contributor

No description provided.

} catch DecodingError.rawRepresentableInitializationError(_, _) {
//then
XCTAssertTrue(true)
} catch {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not catch let error and then change the output to indicate the incorrect exception being thrown?

//given
let anyObject = "1970-01-01T00:00:00Z"
//when
let date = try! Date.decoder(anyObject)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Tests should use .decode() method instead of .decoder()

@@ -98,3 +98,15 @@ extension NSArray: DynamicDecodable {
}

}


extension URL: DynamicDecodable {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should inherit from Decodable as well so that the decode() is available.

let anyObject = ""
//when
do {
_ = try URL.decoder(anyObject)
Copy link
Collaborator

@voidref voidref Nov 5, 2016

Choose a reason for hiding this comment

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

If we are testing the Date decoding, we should probably use the Date object. ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoopsie 😅 . I should've been clearer mentally while writing this.

I'll make the changes you've suggested soon.
Meanwhile, let me just say: decode is 💯

@codeOfRobin
Copy link
Contributor Author

@voidref Any more suggestions?

@voidref
Copy link
Collaborator

voidref commented Nov 5, 2016

@codeOfRobin Just that the calls to decoder should be calls to decode instead. The decoder call isn't really meant to be a public interface, it's just there for customized parameter handling.

@codeOfRobin
Copy link
Contributor Author

@voidref Done! 👍

@voidref
Copy link
Collaborator

voidref commented Nov 6, 2016

LGTM, I am not a maintainer, just a contributor, so you'll need to wait for @Anviking to have time to review and merge. Thanks!

@Anviking
Copy link
Owner

Anviking commented Nov 6, 2016

LGTM too. Thanks, @codeOfRobin, and thanks, @voidref for reviewing!

@Anviking Anviking merged commit 0a59e61 into Anviking:master Nov 6, 2016
@Anviking
Copy link
Owner

Anviking commented Nov 6, 2016

@voidref I just invited you as a repo collaborator also in case you want to!

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.

3 participants