-
Notifications
You must be signed in to change notification settings - Fork 9
Wildcards in mock data file names #42
Wildcards in mock data file names #42
Conversation
Example/Tests/HttpFileParsingTests.m
Outdated
XCTAssertEqual(response.statusCode, kHTTPStatusCodeNotFound); | ||
XCTAssertEqual(data.length, 0); | ||
}]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worthwhile to have a XCTAssertNil(error);
here?
@@ -94,6 +106,10 @@ with the fallback base name | |||
``` | |||
POST|-login-|169d720631e603967135cfce10d235e94aac22b87500ea09d1be295f5b300dca | |||
``` | |||
and an optional wildcard base name | |||
``` | |||
POST|-login-|* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if wildcards are enabled, this will cause anything that would hit the POST|-login-
endpoint, with any data, to get the same file? What happens if you want to for example say "I want the username to be x but the password can be a wildcard?" Thinking of this in relation to something where dates can be whatever but where other things you want specified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the wildcard will allow any query (or body) if enabled. This coarse grained wildcard implementation was the quickest/easiest thing to do for the problem I have (mocking requests with dates in the query). I'm hoping that the cases where you have similar requests that need wildcards can be differentiated in other ways than the query or body. (e.g.: with a unique id in the path GET /users/1234
).
I think that adding more fine-grained wildcarding capabilities goes against the simple philosophy of this library
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Might be worth noting this in the README since it's not 100% clear the wildcards are this coarse-grained.
@vokal-isaac @bryanluby any more thoughts? |
@vokal-isaac @bryanluby any further thoughts on this? |
VOKMockUrlProtocol.m
Outdated
|
||
if (AllowsWildcard) { | ||
[resourceNames addObject:[[name stringByAppendingFormat:AppendSeparatorFormat, Wildcard] mutableCopy]]; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, at this point, the ordered list of possible names is something like:
…|queryString|bodyString
…|queryString|bodyHash
…|queryString|*
…|queryHash|bodyString
…|queryHash|bodyHash
…|queryHash|*
…|*|bodyString
…|*|bodyHash
…|*|*
It seems to me like this might allow a wildcard resource to have higher precedence than a non-wildcard resource, if one mixes query strings and query hashes, but I'm not entirely sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's the ordering you'd get for a request with a query and a body. If you'd like, I can change it to force all the wildcard variations to the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that'd be better... but I'm also wondering if the logic for assembling the various possible resource names might be simplified by, rather than building up the resource name strings as we go along, building up arrays of the possible values for each part, then assembling all possible combinations at the end, and inserting wildcards then as appropriate.
@vokal-isaac, better? |
Bump. @vokal-isaac @bryanluby |
LGTM - waiting on @vokal-isaac |
@vokal-isaac to the code review courtesy phone plz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Today is the first day in weeks (months?) that I've gotten to look at iOS/macOS code.
@chillpop @designatednerd tagged and |
@vokal-isaac glad to see you back on the light side 😄 Thank you! |
Thanks! |
This allows more easily mocking URL requests that include dynamic data such as dates.
@vokal-isaac @designatednerd @bryanluby @lindelldigital, review?
(No rush, I won't be checking comments for about a week)