Convert Tests to Specs.

[tiennou]

Here are rewritten tests (and some new ones I think, see if you can spot them ;-)) using Specta/Expecta. I've only left GTObjectTest because I'm not sure what I should salvage yet because 3 tests on 5 are actually testing GTRepository...

[tiennou]

This needs #280 before the clone test will go green.

[alanjrogers]

Can we also document the header for this method? We should specify that data and repository may not be nil

[alanjrogers]

May not be nil.

[tiennou]

Hmm, how about we agree on defaulting to "unspecified" => "mandatory argument" ? I'm not fond of having to go through every docblock out there and add that info, and the "optional" argument case is much less frequent.

[dannygreg]

There are actually relatively few methods which assert paramaters as not being nil, throughout the codebase.

If something will throw an exception if you pass in nil it should absolutely be documented.

[jspahrsummers]

I can see both sides of this argument, but ObjectiveGit is probably the wrong example to look at here, since it's kinda crufty in many places.

I'm 👍 on @tiennou's suggestion (for all our OSS), though it'll take some time before ObjectiveGit's documentation actually gets to a state where it matches our expectations.

[alanjrogers]

👍 I'm fine with that.

[dannygreg]

It seems incredibly backwards to me that we wouldn't document something which throws an exception.

[alanjrogers]

@tiennou I just started re-reviewing this without checking you were done, sorry about that! Carry on 💃 .

[tiennou]

I'm the sorry one here — it was mostly ready, except for the lone fixup! commit I pushed by mistake :-S. It's fixed now so you can proceed.

[alanjrogers]

Can we rename this method to -lookupObjectByRevParse:error:.

That way it's less likely to be confused with a Refspec.

[alanjrogers]

🌴 Just one comment left :)

[tiennou]

I've done the refspec renaming somewhere else. If that one's merged I got a bunch of others to review, which I'll push ASAP (my net access isn't good at the moment).

[alanjrogers]

@tiennou It looks like that PR won't get merged quickly as it's a lot of changes. Since we're renaming -lookupObjectByRefspec:error: here anyway, it would be good to give it a better name while we're at it.

[tiennou]