Support bit mask on the RKRequestMethod method param

[dulacp]

Is there a reason why you didn't use a bitwise aware enum that could let us do things like :

RKRoute *createUpdateDeleteRoute = [RKRoute routeWithClass:[Album class]
                                               pathPattern:@"/api/album/:albumID/"
                                                    method:(RKRequestMethodGET | RKRequestMethodPUT | RKRequestMethodDELETE)];

Just to save us from duplicate lines ;)

[blakewatters]

Yeah I have thought about this recently. Should definitely make the change -- should be able to do so transparently

[dulacp]

Allright, thanks for the fast answer that's comforting me on making a pull request (and keep it transparently of course)

[dulacp]

Oh! I'm facing some design issues... the fact is that if we store the bitmask method value in the route.method property then there is no way to make the RKStringFromRequestMethod function work :/ because it's mapping one value to one string.

So one solution would be to have a new class method on RKRoute that returns an array of routes, but that's not very transparent. What do you think ?

[blakewatters]

You should still be able to do an exact value comparison with a bitmask. On the request/response descriptors it would make sense to use them as a bit mask, but in routes and other situations they will need to be pinned to an exact value and raise an NSInvalidArgumentException when used inappropriately.

[dulacp]

Totally agree, we can make exact value comparison of course.

On the request/response descriptors it would make sense to use them as a bit mask

Can't find where RKRequestMethod values are used for request/response descriptors... could you give me a hint ?

[blakewatters]

Ah it's actually not currently in use on the descriptors -- I have a branch where I had introduced it as a new option that is unmerged. Had forgotten

[dulacp]

Nice! I'm reassured, I was doubting of my searching skill ;)
So I had to wait for this one ? otherwise I will violate the "A single development branch should represent changes related to a single issue" principle...
Or maybe this issue can be part of the branch you are talking about ?

[blakewatters]

Just go ahead with your development and shoot a pull request over whenever you are ready. I'll take care of integration with any other branches

[dulacp]

Fine with me, I'll do that. Thanks for the hints

[blakewatters]

Looks good -- the only other change I would make is to introduce RKRequestMethodAny, which should have all the bits flipped so that it will match any value. This is used within the routing layer to define a route that matches any method.

[dulacp]

Thanks!
Good idea for the RKRequestMethodAny, I'm thinking of two ways

first, declare that at the end of the enum

RKRequestMethodAny = (RKRequestMethodGET |
                      RKRequestMethodPOST |
                      RKRequestMethodPUT |
                      RKRequestMethodDELETE |
                      RKRequestMethodHEAD |
                      RKRequestMethodPATCH |
                      RKRequestMethodOPTIONS)

or second method, less explicit

RKRequestMethodAny = ~(0)

got a preference ?

[segiddins]

Go with the more explicit option

[blakewatters]

Well the advantage of the second one would be that it would automatically include any additional methods defined, though I feel like the explicit option is easy to read.

[dulacp]

Exactly, it would be more robust for future additions but harder to read, that's why I was asking for a preference.
On second thoughts I will probably go with the explicit option since adding methods will be a very rare case in the future I think, and @segiddins gave his preference.

[blakewatters]

Sold.

[segiddins]

Shouldn't this return false?

[dulacp]

Humm no, what I meant by RKIsAnExactRequestMethodMatch is that the given HTTP method passed to the function is equal to one and only one of the RKRequestMethod options.
The idea is to check if the method is a power of two, or equals to RKRequestMethodAny.

Does that make sense ?

[segiddins]

But RKRequestMethodAny is a mask representing all request methods (so it's not exact)

[dulacp]

Right, that's why I've added this condition
We can rename the static function if that's unclear of course

[segiddins]

I guess that just doesn't jibe with my interpretation of the word exact

[blakewatters]

I agree with @segiddins -- RKRequestMethodANY should not be considered an exact method (nor should RKRequestMethodInvalid. What I am looking for in the conditions is that the given value corresponds to exactly one valid HTTP request method.

Maybe a better name is RKAssertRequestMethodSpecifiesHTTPMethod()

[dulacp]

I can rename it RKIsMatchingASingleRequestMethodOption if you want to ?

[blakewatters]

Sorry another revision: It is valid to use an the method as a bit mask for routing purposes with:

+ (instancetype)routeWithClass:(Class)objectClass pathPattern:(NSString *)pathPattern method:(RKRequestMethod)method;
+ (instancetype)routeWithRelationshipName:(NSString *)name objectClass:(Class)objectClass pathPattern:(NSString *)pathPattern method:(RKRequestMethod)method;

This is because these methods are matched by HTTP method to URL, while named routes go from a name -> URL and method.

If multiple routes are registered that overlap and the value of one is not RKRequestMethodAny then an exception should be raised. The logic being that you can register a wildcard route that is a fallback in the event that there is not a more specific route in place.

[segiddins]

This will also make #1439 a lot easier to implement

[blakewatters]

Did some additional cleanup and merged to development in c73ac51

[dulacp]

Great! glad to here that

[segiddins]

some things we might have missed:
- (NSArray *)enqueuedObjectRequestOperationsWithMethod:(RKRequestMethod)method matchingPathPattern:(NSString *)pathPattern does not match against the bitmask
RKStringDescribingRequestMethod should be public, as it will need to be used for the description of RKRequestDescriptor and RKResponseDescriptor

[blakewatters]

Taking care of the enqueuedObjectRequestOperationsWithMethod:matchingPathPattern: gap now.