Skip to content
This repository has been archived by the owner on May 17, 2024. It is now read-only.

Map from RawRepresentable Enum arrays #34

Closed
wants to merge 3 commits into from
Closed

Map from RawRepresentable Enum arrays #34

wants to merge 3 commits into from

Conversation

litso
Copy link

@litso litso commented Feb 18, 2016

Allow mapping from arrays of enums. For example:

enum Value: String {
  case A = "a"
  case B = "b"
  case C = "c"
}

let json: NSDictionary = ["values": ["a", "b", "c", "a"]]
let map = Mapper(JSON: json)

let values: [Value] = map.from("values")

@keith
Copy link
Contributor

keith commented Feb 18, 2016

Thanks for adding this right when I was having a conversation about this! There's one big non obvious issue we need to deal with here. Let me pull together an example.

@keith
Copy link
Contributor

keith commented Feb 18, 2016

So the issue here is backwards compatibility. We've already gotten bitten by this once. So the case is pretty simple, but it works like this:

Version 1 of your model:

enum Value: String {
  case A = "a"
}

let json: NSDictionary = ["values": ["a"]]

Then you update to version 2 of your model:

enum Value: String {
  case A = "a"
  case B = "b"
}

let json: NSDictionary = ["values": ["a", "b"]]

But then, if you're forcing the values on your original model, the values you're getting from the server are now incompatible (version 1):

enum Value: String {
  case A = "a"
}

let json: NSDictionary = ["values": ["a", "b"]]

I think that we need to expose this as much as possible to consumers so it makes it as difficult as possible to shoot yourself in the foot. Currently you can also have this problem with a single enum property as well, if you don't fallback to some default value.

@litso
Copy link
Author

litso commented Feb 18, 2016

Hmm yes agree this is a great way to self-inflict foot wounds.
Maybe the solution is to change:

public func from<T: RawRepresentable>(field: String) throws -> [T]

..to accept a default value? So the above becomes:
public func from<T: RawRepresentable>(field: String, defaultValue value: T) throws -> [T]

?

@keith
Copy link
Contributor

keith commented Feb 18, 2016

I think we should push this concern to the implementer instead. I think that often (like the case in my example) you won't have a sensible default value for this. And really what you'll want to do is flatMap out the invalid ones. Meaning the signature would be:

public func from<T: RawRepresentable>(field: String) throws -> [T?]

@litso
Copy link
Author

litso commented Feb 18, 2016

Sounds good @keith. Would you like me to amend the PR with those changes?

@keith
Copy link
Contributor

keith commented Feb 18, 2016

That would be fantastic!

Prevents situtations where introducing new values will cause parsing to
fail.
@litso
Copy link
Author

litso commented Feb 18, 2016

@keith pushed up the discussed changes.

The only caveat is that if you don't specify that you are expecting an array of optionals for example by adding flatMap to the end:

struct Test: Mappable {
   let values: [Value]
   init(map: Mapper) throws {
      self.values = try map.from("values").flatMap { $0 }
   }
}

So for example in this instance if you forgot to add the flat map:

struct Test: Mappable {
   let values: [Value]
   init(map: Mapper) throws {
      self.values = try map.from("values")
   }
}

Then this ends up getting called (and parsing fails):

public func from<T>(field: String) throws -> T

@Reflejo
Copy link

Reflejo commented Feb 18, 2016

Seems like we either:

a) Add a new method

public func from<T: RawRepresentable>(field: String) throws -> [T]

or
b) We only have

public func from<T: RawRepresentable>(field: String) throws -> [T]

and we flatMap inside that fc

@keith
Copy link
Contributor

keith commented Feb 18, 2016

I'm surprised that a function that returns [T?] would work when you tried to assign it to your [Value] array? Shouldn't that produce a compiler error?

@litso
Copy link
Author

litso commented Feb 18, 2016

@keith the issue is that it is calling public func from<T>(field: String) throws -> T instead

@keith
Copy link
Contributor

keith commented Feb 18, 2016

Ah yes. That will be going away as soon as I finish my branch of #7

@litso
Copy link
Author

litso commented Mar 14, 2016

@keith i'm assuming this is on hold then until #7 is done?

@keith
Copy link
Contributor

keith commented Mar 14, 2016

Yea sorry, I'll try to get to that soon. It's mostly done, just need some final testing

@keith keith mentioned this pull request May 7, 2016
@keith
Copy link
Contributor

keith commented Jul 11, 2016

Looping back here (a few months later than I would have liked). I have a PR up for the other change here: #59

@keith
Copy link
Contributor

keith commented Jul 12, 2016

Thanks for getting this started! I've submitted #61 to implement this after the new changes.

@keith keith closed this Jul 12, 2016
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.

3 participants