Add Objective-C generic types to collections in headers #11
Conversation
|
Generic type specifiers are really for bridging with Swift, so they would make in headers of GitUpKit but not at every variable declaration site: it just makes variable declarations harder to read IMO. |
|
Ok, I can go back and remove them from everywhere but the headers. I assumed there was more type checking the compiler could do when they were added elsewhere. |
|
Lol, yeah, I noticed the README update. I actually made this change in a number of commits, but squashed them all because I thought that was the preferred way to do things. Would you prefer me to break this up into multiple commits, maybe one for each subdirectory? |
|
It's about balance: 1 change = 1 commit but not to the expense of the diff not being review-able 😄 I don't know what the right thing here is, but first: is it solving an actual problem? If so what problem exactly? And then what is the best solution and why? |
|
You're right that the main benefit of generics in Objective-C is for bridging to Swift. But as the warnings that popped up with the unit tests show, the extra type information can help in Objective-C development as well. I think the fact that this only exposed 2 small warnings in a test file is a testament to how well written the code already is, but it's nice to have the compiler doing these extra checks. I guess it's a level of defensive programming. |
|
I just read a few blog posts, and I didn't realize that the generic syntax in method implementations had no effect. I'll go ahead and remove those, and separate out addition of the types to the headers from the call sites into different commits. I can make this PR focused on the headers only. |
JrGoodle
changed the title
|
I made a branch with independent commits for each class (similar to how nullability annotations were added to ObjectiveGit on a class-per-commit basis). https://github.com/JrGoodle/GitUp/commits/generics-headers If you like how that looks, I can update this PR with that format. |
| - (NSArray*)listAllBranches:(NSError**)error; // git branch -a | ||
| - (NSArray<GCLocalBranch*>*)listLocalBranches:(NSError**)error; // git branch | ||
| - (NSArray<GCRemoteBranch*>*)listRemoteBranches:(NSError**)error; // git branch -r | ||
| - (NSArray<__kindof GCBranch*>*)listAllBranches:(NSError**)error; // git branch -a |
There was a problem hiding this comment.
Since the array can contain GCLocalBranch or GCRemoteBranch, I used __kindof to indicate that the returned array can contain a subclass of GCBranch.
|
You have 20 files touched now, a much smaller diff, and it's a single change: all header files, so no need to split further imo. You missed a .m file though, so double check again. |
|
Nice catch, I removed the changes to one .m file. There are 2 that required modifications to silence warnings, |
|
This whole thing seems fragile: why are warnings showing up in some cases and not others? It's better to wait until Xcode 7 is final. In any case, I don't see a point of doing this change right now. It's just adding complexity to prototypes for no immediate benefits. The real work here is to do a pass on all headers, add such declarations along with all the other ones required for efficient Swift bridging e.g. nullables. But there's no point in doing any of this until there are enough Swift users of GitUpKit and it comes with a Swift example. |
I went through the code base and added support for the new generics introduced in Objective-C in Xcode 7 beta. I'm guessing this probably would be best to merge after the official Xcode 7 release (September 9?), but I wanted to get some feedback on it to make sure everything looks good.
After adding the type specifiers, a couple warnings popped up in one of the test cases where
GCHistoryRemoteBranchwas being assignedGCHistoryLocalBranchobjects, so I changed those to the proper type.