[Immutable] ImmutableMappable #592

Merged
merged 4 commits into from Sep 28, 2016

Conversation

Projects
None yet
2 participants
@devxoul
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

This comment has been minimized.

Show comment
Hide comment
@devxoul

devxoul Sep 23, 2016

Contributor

Why tests fail?

Contributor

devxoul commented Sep 23, 2016

Why tests fail?

@tristanhimmelman

This comment has been minimized.

Show comment
Hide comment
@tristanhimmelman

tristanhimmelman Sep 23, 2016

Contributor

@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.
Contributor

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

This comment has been minimized.

Show comment
Hide comment
@devxoul

devxoul Sep 23, 2016

Contributor

@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.
Contributor

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 referenced this pull request Sep 28, 2016

Merged

Devxoul immutable #594

@tristanhimmelman tristanhimmelman changed the base branch from master to immutable-mappable Sep 28, 2016

@tristanhimmelman tristanhimmelman merged commit 70822e6 into Hearst-DD:immutable-mappable Sep 28, 2016

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 referenced this pull request Oct 3, 2016

Merged

Immutable mappable #608

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