Skip invalid data instead of exiting completely v2

[acecilia]

Hello guys!

I was about to make a pull request exactly the same as #41, so I read the discussion and modified the existing code of @basememara, but following other approach to arrive to the same goal.
(I am kind of newbie to this PR staff, I think this is the proper way of modifying an existing PR, otherwise tell me please how to proceed)

The main change of this PR is done in fb9539b, the other changes are to separate public API from logic, update tests and more.

Check it please.

[basememara]

Hi @acecilia, thx for kick starting this again. Nice touches, like the shortened names and removing the unnecessary returns in the maps.

Also, I see you removed the allowInvalidElements parameter. So am I correct in understanding that it is skipping invalid data by just inferring it instead of an explicit parameter? So to return an empty result if something fails you'd do let items2: [Model]? = Unbox(data), and to just skip invalid and return what's good you'd do let items: [Model] = Unbox(data)?

If that's the case, I think that's really clean; leveraging inference over extra parameters 👍. Maybe too much to imply, but I still think it's really elegant. What do you think @JohnSundell?

[acecilia]

Yes @basememara , that's the idea

[JohnSundell]

Hey @acecilia and @basememara! Thank you so much for your work on this, and I'm sorry I've been super slow with responding - have been traveling and working like crazy this month :) Will take a look at this over the weekend, promise! 👍

[JohnSundell]

Not so sure about modifying the behavior of unboxing an array, since it can have side effects. For example; if my program depends on complete backend data (in the form of an array), then with this change there would be no way to express that (right?). I think the approach with an explicit parameter both makes the programmer's intent more clear, and also makes the API more expressive. What do you guys think @acecilia, @basememara?

[acecilia]

if my program depends on complete backend data (in the form of an array), then with this change there would be no way to express that (right?)

I understand that when you say "complete backend data" means that you want the complete array, or nil when any of its elements failed.

I think this is possible, let me explain it:

If you unbox to a non optional array:
let items: [Model] = Unbox(data)
This enforce to return a non optional array, so it is like saying "I want some data in the array even if the unboxing of some of its elements fail"

On the other hand, If you unbox to an optional array:
let items2: [Model]? = Unbox(data)
This is like saying "I want the complete array if all of its elements are properly unboxed, otherwise do not return any array". So this is the method to obtain the complete array. Btw, I think this method was working in the same way before this PR happened, I just added the methods to unbox to non-optional arrays.

Is this clear now? Or maybe your question was not about this?

[basememara]

I believe I see @JohnSundell's concern, you don't want to "break" any existing API's. Actually breaking an API would be better, what we're discussing here is changing the implicit behavior.

Everyone doing let items: [Model] = Unbox(data) all of a sudden get data returned with this update, whereas before they were getting all or nothing. This can cause dangerous issues with those already using the API as it was understood before. Because of this, I'm agreeing with John about the explicit parameter or at least another API call for this.

[basememara]

Just an extra comment, if getting data back was that dangerous before for someone, they should've used UnboxOrThrow. Still though, changing implicit behavior is never good unless everyone using it is notified or some kind of compiler warning.

[basememara]

I still like @acecilia's implementation because it's not invasive on the source code. So instead, the explicit parameter can stay on the surface while retaining previous behavior, maybe something like from this:

    static func unbox<T: Unboxable>(dictionaries: [UnboxableDictionary], context: Any? = nil) -> [T]? {
        return try? Unboxer.unboxOrThrow(dictionaries, context: context)
    }

    static func unbox<T: Unboxable>(dictionaries: [UnboxableDictionary], context: Any? = nil) -> [T] {
        return dictionaries.flatMap { unbox($0, context: context) }
    }

To this:

    static func unbox<T: Unboxable>(dictionaries: [UnboxableDictionary], context: Any? = nil, allowInvalidElements: Bool = false) -> [T]? {
        return allowInvalidElements
             ? dictionaries.flatMap { unbox($0, context: context) }
             : try? Unboxer.unboxOrThrow(dictionaries, context: context)
    }

It's the best of both worlds: least changes to source code and backwards-compatibility. Juggling/forking multiple PR's against each other wasn't so obvious like @acecilia mentioned, so sorry but I updated it on the other pull request with this change: #41

However, there's one catch so far that we'll need to iron out, it has to be used like below as of now blagggghhh, but maybe a good milestone to start from tho:

let items: [Model] = Unbox(data, allowInvalidElements: true) ?? []

[acecilia]

@basememara I do not really see the problem, before this PR, doing let items: [Model] = Unbox(data) was causing a compile error:

So it is not changing the behavior. To get an unwrapped array the developer has to unwrap it after using the Unbox() function.

And if someone is doing if let items: [Model] = Unbox(data) {} I think that still works the same as before with this PR: only gets into the if when all the elements in the array are properly unboxed (compiler still calls the Unbox() function with an optional return parameter, not changing the behavior)

Right? I mean, i still do not see the problem. I understand that adding a parameter is easier to follow, but if we are returning an array with failable elements there is no sense to return an optional (in case we finally implement an Unbox() function with a parameter and returning an optional value), and I think using it this way makes sense.

[basememara]

Hmmm you're right! This never worked before: let items: [Model] = Unbox(data)

@JohnSundell can you elaborate on your concern? @acecilia's change wouldn't break any previous behaviors and the inference seems more concise.

[JohnSundell]

Sorry if my first comment wasn't very clear - indeed I meant arrays of dictionaries when I said "backend data". While I really like using type inference to also infer behavior, I feel like in this case it can become a bit ambigious. There are really four cases here;

1. I want only a complete array, and it's required - if an element failed to unbox - fail the whole process.
2. I want only a complete array, and it's optional - if an element failed to unbox - return nil.
3. I'm fine with a partial array, if an element failed to unbox - skip it.

The forth case can only occur if the array is nested in another structure;

4. I'm fine with a partial array, as long as it's an array, and if an element failed to unbox - skip it.

[JohnSundell]

I took the liberty of writing an implementation myself (V3) :) that uses an explicit parameter (that defaults to false to be consistent with the current behavior) - and has less impact on the code base thanks to the use of an Array extension; what do you guys think? #50

[acecilia]

Perfect, like it

[JohnSundell]

@acecilia Awesome :) Thanks so much to both you and @basememara for your work on this 🙇

[basememara]

I really like it too, nice to use an array extension to put that logic. Glad we can help, thx for an awesome library and being open to our suggestions :)

[JohnSundell]

@basememara Of course, your awesome suggestions is what made this happen! 👍

[JohnSundell]

And if you have any other improvement suggestions or changes you'd like to make, please keep the PRs coming! 🚀