add nil protection for NSSet map method

[orta]

This has caused me two crashes in beta, figured it was time to get it fixed.

[marcpalmer]

Definitely don't compact. The nils in collections problem is a standard iOS idiom so stick with it IMO. Are there any existing collection methods in the SDK that do some kind of mutation and have to deal with this? There may be a precedent for crash vs. injecting NSNull.

I vote for being "platform consistent".

[supermarin]

So @mattt brought up a good point: it should keep the same behavior as if you called -valueForKeyPath:.

NSDictionary *person1 = @{ @"name": @"Marin" };
NSDictionary *person2 = @{ @"name": @"Luca" };
NSDictionary *person3 = @{ @"age": @12 };
NSDictionary *person4 = @{ @"name": @"John" };

NSArray *names = [@[person1, person2, person3, person4] valueForKeyPath:@"name"];

(lldb) po names
<__NSArrayI 0x8a4fb20>(
Marin,
Luca,
<null>,
John
)

In this case, -valueForKeyPath: inserts [NSNull null] objects and doesn't blow up.

I'm mostly now biased for making this as a default behavior. Thinking of -map:withOptions, still not sure about it, since you'd be able to chain map and compact to get the clean array back.

[[oneToTen map:^id(NSNumber *numbah) {
    BOOL isEven = numbah.integerValue %2 == 0;
    return isEven ? numbah : nil;
}] compact];

vs

[[oneToTen map:^id(NSNumber *numbah) {
    BOOL isEven = numbah.integerValue %2 == 0;
    return isEven ? numbah : nil;
}] withOptions:OSMapOptionsCollapseNil];

Thoughts?

[dblock]

I find withOptions very hacky.

[neilco]

I agree that withOptions is hacky. Let's keep the behaviour consistent with the system APIs.

[marcpalmer]

Sounds sensible to me. Not withOptionsShmoptions.

[orta]

I'm happy to make the change, but in doing so I'll have to make changes to NSArray that are API breaking. This probably means switching the version to 2.x.x wrt SemVer.

[orta]

poke about the fact that this means a major version bump

[supermarin]

@orta @dblock yesterday I went to fix this. And, the story doesn't end here.

NSSet -valueForKeyPath: will ignore nils NSArray -valueForKeyPath: will put NSNulls

Seems like people don't see that as a positive thing, and they expect the APIs to behave the same. My current state of thought is that they should both ignore nils.

If you need counts for something, there's nothing useful in presenting a NSNull. If you call any method on it, it'll blow up.

[orta]

as someone put it on twitter, one has different expectations for sets vs arrays. I think the answer here is to allow set to have a nil, and array to use nsnulls.

[orta]

closing, as this is pretty much a hung PR.