Skip to content
This repository has been archived by the owner on Sep 19, 2018. It is now read-only.

Adoption of throws, remove Result dependency #50

Merged
merged 38 commits into from
Oct 30, 2015

Conversation

zwaldowski
Copy link
Contributor

This is it… the big one. This completes the triumvirate of Swift 2 PRs, starting with #48 and #53.

This is a big breaking change and should be considered very carefully for completeness, clarity, and documentation coverage.

  • Introduces a new subscripting API that throws on error
  • Moves to throws for deserializing, serializing, parsing, and encoding.
  • Moves the act of deserializing JSON into the two JSON parsers to a protocol.
    • Gives JSONParser a public API.

TODO/Known issues:

  • Update README.

@zwaldowski
Copy link
Contributor Author

Updated to depend on #53.

@zwaldowski
Copy link
Contributor Author

Rebased to resolve all dependencies.

@zwaldowski
Copy link
Contributor Author

Further changes have been made to subscripting to reduce complexity and remove more code.

  • String, Bool, Int, and Double now themselves conform to JSONDecodable.
  • Exposes a decode(_:type:) family of subscripts that work for any JSONDecodable. For an implementer of JSONDecodable where your init(json:) is primarily assigning to stored properties, this means 99% of your code could go through decode(_:type:).
  • Exposes a arrayOf(_:type:) family of subscripts that combine array(_:) and decode(_:type:).

Please give another glance over JSONSubscripting if already have.

@jjmanton jjmanton self-assigned this Oct 7, 2015
@zwaldowski
Copy link
Contributor Author

Updated to replace JSONPathType with a concrete JSON.PathFragment. This involves a little bit of funniness with literal convertibles (:car:) and default arguments, but testing against #59 yields a 15% performance increase to decoding.

}

let key = try! decodeString().string()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you using try! and not just try? If the decodeString() throws, don't we want to propagate that error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very good point. I think the change passes just happened at different times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, it might also be that I was going to change the internal functions like decodeString() to return String instead of JSON. But that'll be later.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is one of the few things that I saw in all of this that I think is "wrong". As far as I can recall, most of the other suggestions are simply thoughts/suggestions/confusion.

If this should be changed, go ahead and make it, or I can if you're ok with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

@zwaldowski zwaldowski changed the title RFC: Adoption of throws, remove Result dependency Adoption of throws, remove Result dependency Oct 30, 2015
@zwaldowski zwaldowski force-pushed the zwaldowski/swift-2_0/throwification branch from eecddb9 to 874ea3a Compare October 30, 2015 02:36
@zwaldowski
Copy link
Contributor Author

Updated to address comments as well as resolve mergability with its base branch.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants