Added correct way of default parameters#804
Conversation
|
Should I see if I can come up with some tests to test it works well? |
designatednerd
left a comment
designatednerd
left a comment
There was a problem hiding this comment.
@kimdv Yes tests would be awesome on this. Thanks!
|
@designatednerd added some tests. Don't know if it is enough? Also changed name of test file to be more explicit as it also tests custom and requestBody now, Let me hear what you think 🚀 |
| func testEncodingQueryWithNullDefaultParameter() { | ||
| let operation = HeroNameQuery() | ||
| let body = requestCreator.requestBody(for: operation) | ||
| let body = requestCreator.requestBody(for: operation, sendOperationIdentifiers: false) |
There was a problem hiding this comment.
Probably worth adding one test to this file where sendOperationIdentifers is true just to make sure that's actually including the operation identifier
There was a problem hiding this comment.
Well, the default implementation here was actually false.
Maybe that is for another PR?
I can open an issue to you can track it?
There was a problem hiding this comment.
Just not to add something that is not related to this PR and RequestCreator? 😁
There was a problem hiding this comment.
Yeah I'll add it in a follow-up PR - you don't need to open an issue for it, I'll leave this comment open
|
|
||
| let data = try customRequestCreator.requestMultipartFormData( | ||
| for: HeroNameQuery(), | ||
| files: [alphaFile!], |
There was a problem hiding this comment.
Probably worth guard let ing this on creation, since a force-unwrap failing in a test causes the whole test suite to crash rather than failing the single test.
There was a problem hiding this comment.
This is just copy/paste from other code made by you 🙈
Should I remove all force unwrap in this class?
There was a problem hiding this comment.
🤦♀️Nah I'll fix it in a follow-up PR.
There was a problem hiding this comment.
Haha, I'm the moron who did it first, no worries
designatednerd
left a comment
designatednerd
left a comment
There was a problem hiding this comment.
LGTM pending CI. I'll do a follow up-PR once this is merged to do some test cleanup
|
Ping me if you need help with clean up 😁 |
|
@kimdv Tests seem to have bombed out on MacOS - can you double check that the files are added to the proper targets? |
|
@designatednerd struct was fileprivate 🤦♂️ |
| /// - Parameters: | ||
| /// - string: The string to append | ||
| /// - name: The name of the part to pass along to the server | ||
| public func appendPart(string: String, name: String) throws { |
There was a problem hiding this comment.
@designatednerd this was only internal. The TestCustomRequestCreator used it and I know I use in my project too.
https://github.com/apollographql/apollo-ios/pull/804/files#diff-b1b6d9ae6149908bf9768bbc4bc4b3b0R50
… cannot rely on the order
|
Wow. I saw the comment that the there is a weird order. But it took some time to find out why! |
|
Still approved! |
2 participants
How I missed that I don't know..
Because the
HTTPNetworkTransportdidn't usemanualBoundaryit will use de protocol extension and not the requestCreator that is provided and implements the method.I think this is the best way to implement "default" parameters