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

[Immutable] ImmutableMappable #592

Merged

Conversation

@devxoul
Copy link
Contributor

devxoul commented Sep 23, 2016

Related to #383 and #384

Summary

  • Supports Swift 3
  • This PR doesn't break existing implementation.
  • Adds ImmutableMappable to support immutables.
  • ImmutableMappable has init(map:) throws.
  • ImmutableMappable makes func mapping(map:) optional; basically supports transforming from JSON to Immutable object. Users needs to implement this function to support reverse transform (object to JSON).
    • init(map:) throws: JSON -> Object
    • mapping(map:): Object -> JSON

Example

User

struct User: ImmutableMappable {
  let name: String
  let createdAt: Date
  let updatedAt: Date?
  let posts: [Post]

  // JSON -> Object
  init(map: Map) throws {
    name = try map.value("name") // throws an error if fails
    createdAt = try map.value("createdAt", using: DateTransform()) // throws an error if fails
    updatedAt = try? map.value("updatedAt", using: DateTransform()) // optional
    posts = (try? map.value("posts")) ?? [] // optional + default value
  }

  // Optional: Object -> JSON
  mutating func mapping(map: Map) {
    guard case .toJSON = map.mappingType else { return }

    var name = self.name
    var createdAt = self.createdAt
    var updatedAt = self.updatedAt
    var posts = self.posts

    name <- map["name"]
    createdAt <- (map["createdAt"], DateTransform())
    updatedAt <- (map["updatedAt"], DateTransform())
    posts <- map["posts"]
  }
}

Usage

let user = try User(JSONString: JSONString)

Discussion

  • Will it better to provide toJSON(map:) or something else instead of mapping(map:) for ImmutableMappables?
@devxoul
Copy link
Contributor Author

devxoul commented Sep 23, 2016

Why tests fail?

@tristanhimmelman
Copy link
Owner

tristanhimmelman commented Sep 23, 2016

@devxoul sometimes I have to restart some of the tests in Travis for them to succeed... I have restarted the two that failed, I think they will probably resolve themselves.

From a quick glance your work looks good, thanks for contributing 👍. I will take some more time soon to look at it closely. Here are a couple things I'm considering:

  1. Should we make the mapping function optional?
  2. How could we improving the toJSON functionality for immutable variables
  3. Do we need a separate protocol form ImmutableMappable? I do like that this doesn't break any current functionality, but I wonder if it would just be simpler to have init(map: Map) throws as the main init protocol.
@devxoul
Copy link
Contributor Author

devxoul commented Sep 23, 2016

@tristanhimmelman, thanks for your review! These are my answer:

  1. Mappable should require func mapping(map:) because this is the only function which defines mapping relationship (init() is not designated for mapping). However, ImmutableMappable has an initializer for doing that. I thought it was OK to not implement the func mapping(map:) if you're not using toJSON functionality.
  2. Well, we could imagine something like map["key"] <- self.property instead of self.property <- map["key"] to get rid of declaring local mutable variables. But neither can this remove the code repetition :-(
  3. I really want to make the initializer of Mappable throwable. Why I made ImmutableMappable separately is because I thought you were worrying about the breaking API changes. How about using ImmutableMappable as an experimental feature? We can improve the detailed implementation in future release while giving users an opportunity to use immutables earlier.
@devxoul devxoul force-pushed the devxoul:immutable branch from 25ce06d to 6825fe5 Sep 23, 2016
@devxoul devxoul force-pushed the devxoul:immutable branch from 6825fe5 to 0e4a4d5 Sep 23, 2016
@devxoul devxoul force-pushed the devxoul:immutable branch from 0e4a4d5 to 10debd9 Sep 23, 2016
@devxoul devxoul mentioned this pull request Sep 28, 2016
@tristanhimmelman tristanhimmelman changed the base branch from master to immutable-mappable Sep 28, 2016
@tristanhimmelman tristanhimmelman merged commit 70822e6 into tristanhimmelman:immutable-mappable Sep 28, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@devxoul devxoul deleted the devxoul:immutable branch Sep 30, 2016
@devxoul devxoul mentioned this pull request Oct 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.