Fixed #37.

[ashfurrow]

No description provided.

[ashfurrow]

Yeah actually this doesn't make sense. Hold up, yo.

[ashfurrow]

That's better.

[dblock]

I am confused by the name of this variable: "withinBounds"? Is that more like "hasOriginalSize"?

[ashfurrow]

Yeah that's better 👍

[dblock]

Also, when contentSize is zero, you get zero. That's possible, right? and would be assertable?

[ashfurrow]

Sure thing.

[ashfurrow]

Fixed those issues.

[dblock]

This definitely needs tests! Since you cannot stub the assert I would extract the asserting code into a function that returns a BOOL and stub it away in a test.

[dblock]

Also please update CHANGELOG.

[ashfurrow]

Ah, would you believe that I did update it but I forgot to add it to the staged changes? >.<

[ashfurrow]

I definitely agree that this should be tested, though I'm not sure that I like the idea of a checking method doing the assertion, instead. Hmm...

[dblock]

Needs an - [@ashfurrow](...), see format below.

[ashfurrow]

👍

[dblock]

What do you think of this? Makes it testable without a weird interface.

- (BOOL)checkContentSize
{
  BOOL hasContentSize = self.hasContentSize;
  NSAssert(hasContentSize, @"originalSize dimension is zero, will result in NaN in returned value.");
  return hasContentSize;
}

- (BOOL)hasContentSize
{
  return fabsf(self.originalSize.width) > 0 && fabsf(self.originalSize.height) > 0;
}

BOOL hasContentSize = [self checkContentSize];
if (...) { 
}

[ashfurrow]

I think we can write a test to expect the raised exception:

it(@"asserts on zoomRelativePoint: with a zero content size", ^{
    expect(^{[mapView zoomRelativePoint:CGPointZero];}).to.raise(@"NSInternalInconsistencyException");
});

[ashfurrow]

What do you think?

[dblock]

I think there're two expectations here to test: one that it raises, but the other that it returns the original point in that case.

[ashfurrow]

That's a valid point – I just don't like the idea that a method that's checking for a condition and returns success/failure might throw. Seems ... unsavoury somehow. Let me think on this.

[orta]

raising an exception and returning the original point is a really weird pattern IMO. I think just raising the exception should be enough, it's to catch programmer errors.

[ashfurrow]

So maybe get rid of the else clause altogether?

[dblock]

I'm cool with what @orta thinks.

This discussion btw would make for a good blog post :)

[ashfurrow]

👍

You don't have to tell me twice.

[ashfurrow]

💚

[dblock]

Should this say "Added assert inside NAMapView#zoomRelativePoint when returning {NaN, NaN}"?

[ashfurrow]

sounds good

[dblock]

k, merged, thanks for your cooperation and patience with me and @orta, @ashfurrow

[ashfurrow]

No problem! Does one of you own the pod on trunk? I'll write up a blog post tomorrow.

[dblock]

I'll do a release at some point soon, for now just use :head.

[ashfurrow]

:head actually just uses the latest podspec version, not the git repo's head. I'll point to a commit hash, instead.

[dblock]

That's nuts.

[orta]

that sounds unintuitive, are you sure that's the behavior? It should do that without the :head

Though it looks like the guides' section on this is broken, sigh.

[ashfurrow]

Hmmm. Now I'm confused. I thought that was the behaviour, based on this stack overflow question and problems I've had with the ReactiveCocoa podspec. Now I'm not so sure.