Convert Tests to Specs. #292
Conversation
…dir` version. Because I don't want to handle the pain of relativizing the passed URL.
I know there's still a test in here, but I'm not sure what it's testing since we're ARC now…
|
This needs #280 before the clone test will go green. |
ghost
assigned 
| @@ -73,6 +76,9 @@ - (id)initWithString:(NSString *)string inRepository:(GTRepository *)repository | |||
| } | |||
|
|
|||
| - (id)initWithData:(NSData *)data inRepository:(GTRepository *)repository error:(NSError **)error { | |||
There was a problem hiding this comment.
Can we also document the header for this method? We should specify that data and repository may not be nil
| // | ||
| // This copies the data from the file to the repository's object database. | ||
| // | ||
| // data - The file to copy contents from. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It seems incredibly backwards to me that we wouldn't document something which throws an exception.
|
@tiennou I just started re-reviewing this without checking you were done, sorry about that! Carry on 💃 . |
Conflicts: Classes/GTDiff.m
|
I'm the sorry one here — it was mostly ready, except for the lone |
| @@ -176,7 +176,7 @@ | |||
| - (id)lookupObjectBySHA:(NSString *)sha error:(NSError **)error; | |||
|
|
|||
| // Lookup an object in the repo using a revparse spec | |||
| - (id)lookupObjectByRefspec:(NSString *)spec error:(NSError **)error; | |||
| - (id)lookupObjectByRevspec:(NSString *)spec error:(NSError **)error; | |||
There was a problem hiding this comment.
Can we rename this method to -lookupObjectByRevParse:error:.
That way it's less likely to be confused with a Refspec.
|
🌴 Just one comment left :) |
|
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). |
|
@tiennou It looks like that PR won't get merged quickly as it's a lot of changes. Since we're renaming |
|
|
Convert Tests to Specs.
Here are rewritten tests (and some new ones I think, see if you can spot them ;-)) using Specta/Expecta. I've only left
GTObjectTestbecause I'm not sure what I should salvage yet because 3 tests on 5 are actually testingGTRepository...