[GTRepository shouldFileBeIgnored:success:error:] cannot be used from Swift

[onecrayon]

This is actually a general problem that affects a few different methods throughout the codebase. The problem is that Swift assumes that the BOOL return value indicates success or failure, and as a result it is impossible to get the return value for this method (and others like it). In order to fix this it would need to be revised to something along these lines (hopefully with better naming):

// Returns YES on success or NO on failure
- (BOOL)shouldFile:(NSURL *)fileURL beIgnored:(BOOL *)ignored error:(NSError **)error;

I'm happy to revise methods that have this problem as I run into them and submit pull requests, but I wasn't sure what would be the best method for handling the change, since it's nowhere near backwards compatible (add a deprecated message for the original method? Just leave it alone and add a wrapper that will work properly in Swift? Also, how should I revise the method name, since I'm not too happy with the pattern that I used above?).

Thoughts?

[tiennou]

I suggest - (BOOL)fileAtURL:(NSURL *)fileURL isIgnored:(BOOL*)ignored error:(NSError **)error ? That mostly-mimics -[NSFileManager fileExistsAtPath:].

[joshaber]

Well that's annoying 😞

I think I prefer -shouldFile:beIgnored:error: since it puts the verb in the first part of the selector, but I don't feel terribly strongly.

[pietbrauer]

👍 for Josh's proposal

[onecrayon]

I've been giving this some thought, and since the primary reason to introduce a new method at all is for use in Swift there's another pattern that might make sense:

// Returns 1 if the file should be ignored, 0 should not be ignored, -1 for failure
- (NSInteger)shouldIgnoreFileURL:(NSURL *)fileURL;

Or alternately use an enum:

typedef NS_ENUM(NSInteger, GTFileIgnoreState) {
    GTFileIgnoreStateIgnoreCheckFailed = -1,
    GTFileIgnoreStateShouldNotIgnore = 0,
    GTFileIgnoreStateShouldIgnore = 1
};
- (GTFileIgnoreState)shouldIgnoreFileURL:(NSURL *)fileURL;

The logic behind this approach is that the success and ignored flags are mutually exclusive, so there's no particular downside to collapsing them into a single return value (although the current method defaults to claiming a file should be ignored if the call to git_status_should_ignore fails, so this would be a departure from the primary method's behavior). Additionally, leaving off the NSError parameter allows much nicer access to this in Swift because you can just toss it in a conditional instead of needing to harvest the return value with a do/try/catch block first.

Plus it removes any question of deprecating the original method, since it would simply be a wrapper that allows slightly different access to the same information.

[joshaber]

I like that a lot 👍 But why would we remove the error parameter?

[onecrayon]

Error parameters make Swift code quite verbose because the bridge automatically strips out the NSError parameter and gives the method the throws keyword (so unlike in Objective-C where I can toss in a NULL if I don't care about the error, in Swift the compiler forces me to use a try/catch block before it will even compile my code). For instance using my proposed method with an enum:

// These are defined previously with the following types
let repo: GTRepository
let file: NSURL

// If the method uses NSError we get this:
var shouldIgnore: GTFileIgnoreState
do {
    try shouldIgnore = repo.shouldIgnoreFileURL(file)
} catch let error as NSError {
    // Log or otherwise handle error
}
if shouldIgnore == .ShouldNotIgnore {
    // Do stuff with file
}

// Without NSError (as originally proposed):
if repo.shouldIgnoreFileURL(file) == .ShouldNotIgnore {
    // Do stuff with the file
}

[joshaber]

Sure, but then you lose any error information.

While I agree that do/try/catch add a lot of syntactic overhead, that seems inherent in Swift. Dropping the error information on this one method to battle that feels like beating against the wind.

Also, I haven't used Swift much, but couldn't you just do:

let shouldIgnore = try? repo.shouldIgnoreFileURL(file)

Granted, then you're dealing with an optional, but it's a little less cumbersome.

[onecrayon]

Nice, I hadn't come across the try? syntax before; that does indeed cut down on the necessary code, and makes me a lot less inclined to strip out the NSError in general.

However, it's apparently the end of my work day and my brain has died, because now that I look at this particular method what actually happens if there's an error parameter is that the bridge cannot translate it (since the return value's not a BOOL or nullable object), and so it comes across to Swift with a signature like this:

shouldIgnoreFileURL(fileURL: NSURL, error: NSErrorPointer)

Not inherently good or bad, as the end result is that you can grab the NSError if you want it or pass nil if you don't (plus there's at least one other method that I've come across in Objective-Git that doesn't bridge with a throws keyword automatically, so although this will introduce some variance to the framework's method pattern in Swift, it wouldn't be the first). If the above method signature looks good to you, I'll toss a pull request your way.

Lastly, any preference for enum vs. NSInteger return? I'm currently using the enum in testing simply because then my code is more self-documenting, but it does introduce yet another one-off enum to the codebase.

[joshaber]

Ha, well, that's a bummer.

Lastly, any preference for enum vs. NSInteger return?

👍 enum.