Skip invalid data instead of exiting completely.

[basememara]

Whether using Unbox or UnboxOrThrow, the current implementation nil's the object or returns an empty collection completely if there is a single property on a single object that is invalid. This is necessary when perfect data is needed.

However, this pull request adds an option to skip invalid data and move on to the next property or object. This way, the consuming developer can prefer to deal with default values instead of an empty collection or nil object.

Use as:

let items: [Post] = UnboxOrSkip(data)

Also part of discussion #39.

[JohnSundell]

You're never using this new parameter.

[basememara]

Good catch, that was a mistake left from previous attempt before going with UnboxOrSkip instead of overloading other functions.

[JohnSundell]

I actually think this is the better approach :) See my comment on the PR itself.

[JohnSundell]

Thanks for this PR! 🚀 I think it's a great addition. 👍

However, I think it'd be a lot better if you continued the parameter approach (extending Unbox() -> [T] rather than introducing a new UnboxOrSkip() function. Adding a lot of new Unbox... main functions will make the API more complicated and harder to maintain, since every time something is changed all the functions have to be updated. And since it's only relevant for arrays anyway, it makes sense to implement it as a parameter.

Also, skipInvalidData kind of sounds like you will skip on an entire invalid array, rather than elements of the array. The intent here is to surpress errors in case an error was encountered while unboxing a member of the array, so how about naming it allowInvalidElements: Bool = true (this matches the Element naming convenion of Array. What do you think?

[basememara]

Yes I actually toyed with both ways, as an overloaded parameter and using a separate UnboxOrSkip. In retrospect, using an overloaded parameter on Unbox seems like the way to go too because it is indeed a parameter and overkill to create a new function. It doesn't change the overall implementation or return types (returning a nil list is better than empty one, it gives more of an indication of what happened).

I never liked skipInvalidData name. I really really like allowInvalidElements.

Let me make the changes and add some unit tests and I'll update the PR. Thanks a mil for the input!

[JohnSundell]

Awesome! Thank you so much for doing this 👍

[basememara]

I just updated the PR to include the updates we discussed and it feels better. The only thing I don't like is I was forced to add the allowInvalidElements parameter to UnboxOrThrow as well so it can be reused for Unbox. I think that defeats the purpose of throwing errors since now what possible errors can UnboxOrThrow throw? In other words, does this even make sense?: UnboxOrThrow(data, allowInvalidElements: true)

I've also added a unit test. I couldn't figure how to reuse your mock data so I just generated dummy JSON and unboxed it.

Let me know if you have any thoughts or suggestions from here and I'll be glad to do another iteration on it. Thanks again!

[basememara]

Ok not sure how you're going to feel about this, but I moved the public API logic into private functions of Unboxer. This way, the public API functions are purely wrappers for them and refactoring can happen in the private functions instead of the public UnboxOrThrow functions. This also resolved UnboxOrThrow having the allowInvalidElements parameter.

[JohnSundell]

Could you move this so that it's together with the other static functions? Also, the documentation can be removed (since it's private), and remove the default value for the context parameter.

[JohnSundell]

Sorry for the delayed response :) I really like that you moved the API logic into private functions - it makes a lot of sense 👍 Some minor comments on the formatting, otherwise - great stuff! The only place where I think we need to make some changes is the test - instead of having a JSON string there I think it's a lot better to create the dictionaries in code. Let me know if you want me to help you out on any of these parts :) Thank you a lot for all the work you've done so far 🚀

[basememara]

Cool, I updated the code based on your comments, except for the testing. Didn't like it either, will take another swing at it..

[JohnSundell]

Awesome 👍

[JohnSundell]

Awesome stuff! Thanks for making those changes! 👍 Now just the testing left then we can merge this one! 😄

[basememara]

Having trouble getting the test to work. Here's what I'm trying:

    func testUnboxWithAllowInvalidElements() {
        struct Model: Unboxable {
            let successProperty: String
            let failedProperty: String

            init(unboxer: Unboxer) {
                self.successProperty = unboxer.unbox("success")
                self.failedProperty = unboxer.unbox("notexist")
            }
        }

        let array: [UnboxableDictionary] = [
            [
                "success" : "Hello"
            ],
            [
                "success" : "Unbox"
            ]
        ]

        guard let items: [Model] = Unbox(array, allowInvalidElements: true) else {
            XCTFail("Could not unbox collections from data")
            return
        }

        XCTAssert(items.count == 2, "Unbox did not return correct number of elements with invalid data")
    }

Commenting out the failedProperty makes it work, but otherwise it returns zero items. Not sure if it's the way I'm using UnboxableDictionary or a bug in my code? I committed it in case you want to run it.

[basememara]

..wait think I found something... hang on..

[basememara]

Ok there was actually a bug in my code. There were a few more places I had to add allowInvalidElements parameter on the dictionary side. Also, had to add it to the single-type returns (instead of just collections) so the variable can be carried all the way through. Good thing you told me to update the tests! I added a test for dictionary and another for json. Let me know if you think of anything else and I'll be glad to take another iteration. Much appreciated!

[JohnSundell]

The method name here should reflect that it takes Dictionaries instead of just Dictionary

[JohnSundell]

Looks much better now! Although I just thought of something - why add the allowInvalidElements to the Unboxer itself? Isn't it better to keep this flag only in the code that deals with arrays, since it's only relevant for arrays? That is, in performUnboxingWithDictionar(ies)AndContext. That would do the job, and make a lot smaller impact on the rest of the code, right?

[basememara]

You mean as opposed to keeping them in the performUnboxingWithData.. functions? I needed them in the data function to parse JSON from web endpoints.

Or if you mean why keep it in the Unboxer class at all, I originally didn't want to, but I needed to hold that flag in memory so it was always aware of that flag. Otherwise, it exited throwIfFailed. If we can get that flag to be available in throwIfFailed, then we wouldn't need to keep it in the class.

[basememara]

I agree tho, removing that property from Unboxer would bring it closer to being a struct one day and immutable. Hmm..

[JohnSundell]

Yeah, you can put it in performUnboxingWithData as well, and from there call the array based one, given an array from NSData, since the flag only needs to be checked if one dictionary in the array failed to unbox. So the logic would be:

- Unbox from data (allowInvalidElements can be set).
- If the data is an array, call the dictionary API for each element (pass allowInvalidElements).
- If Unboxer did throw for one element, check allowInvalidElements before bubbling the error to the main function.

Makes sense?

[basememara]

Trying to follow but not grasping yet, looking at it more..

[JohnSundell]

@basememara Need to go to bed now (It's 00:40 here), but let's catch up tomorrow. Are you on Twitter? Maybe talking about this through IM is going to be easier, for faster turnaround! But I think we are very close to finishing this :) I'm @JohnSundell if you want to catch up on Twitter.

[basememara]

For sure, no rush. Will let it marinate and give it more thought as well. Thanks for the input again.

[basememara]

hey! cool you were right!! I moved allowInvalidElements out of the Unboxer property and scoped it to throwIfFailed as a func parameter. All the tests pass and works with my local app. Can you take a closer look and make sure it jives?

[JohnSundell]

The parameters in this function are placed in the wrong order, since Closure is part of the signature, it should be the first argument.

[basememara]

I actually intentionally swapped them to allow trailing closure syntax.

[JohnSundell]

Ah, I get it. But this is not a public API, so you don't really need that convenience. Also, the name of the method should always match the first argument. So unless you have a strong opinion about it - I think they should be switched.

[basememara]

Right, it's just syntax sugar for internals, which we even aren't using since we're passing variables as parameters anyway. I was only thinking in case someone wanted to extend the library and has that available, but didn't feel strongly about it. Method names matching the 1st parameter is a good convention I wasn't thinking about 👍

[JohnSundell]

Starts to look really good! Sorry for the slow replies, I'm traveling at the moment so have limited time to review PRs :/ Put some comments inline, let me know what you think.

[basememara]

No worries, take your time. We're making great progress and sleeping on it definitely helps 👍

[acecilia]

@basememara why don't you merge my branch in yours instead of downloading the code and committing it? This way of joining the code destroys commits history right?

[basememara]

Yes I agree. I tried merging, creating pull requests, etc but couldn't get it done. I reverted the change just now. I believe from here you have to create a pull request against my repo, I'll accept it, then I'll place my changes on top.