-
Notifications
You must be signed in to change notification settings - Fork 537
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change HttpResponseBody.json to take Any #393
Change HttpResponseBody.json to take Any #393
Conversation
I only noticed just now that there is a similar PR (#390). However, that PR does not add any tests for the new behavior. |
Thanks for adding the tests. |
Hey, @edwinveger Thanks for taking the time to create this! Can you please update the |
@Vkt0r took me a while, but done. |
Hey, @edwinveger thanks! Can you please rebase and clean the branch? |
Generated by 🚫 Danger |
@Vkt0r rebase done. By cleaning the branch, do you mean squashing the commits? Tests are failing to compile on Linux, but I'm not sure how I can fix that. This is my first open-source PR, thanks for your patience 😁 |
Hey, @edwinveger No problem at all 😉. Don't worry about the Linux tests, I've to report this issue to CircleCI with the Docker image. I'll check out your branch in the meantime and I'll run it locally in my machine with Docker in Linux. Can you please squash your commits ? If everything is running for me locally I'll merge it ! |
@edwinveger I checked your PR locally and it's failing with some tests and also I rerun the failed jobs in CircleCI as they have issues as I mentioned before and now the failed jobs reflect the failed tests see details in the PR, please. |
Hey, @edwinveger I was checking out your PR and I figured out what was wrong with it in Linux! We have some compilation conditions for platforms in that specific part of the code and it's not necessary anymore: swifter/XCode/Sources/HttpResponse.swift Lines 34 to 53 in 2f60ae4
The condition is not longer necessary as we can use case .json(let object):
guard JSONSerialization.isValidJSONObject(object) else {
throw SerializationError.invalidObject
}
let data = try JSONSerialization.data(withJSONObject: object)
return (data.count, {
try $0.write(data)
}) Would make your tests pass on macOS and Linux 😄. Can you please update your PR to reflect this change ? |
Thanks for the research. I’m on holiday now, I’ll pick this up when I’m back (end of May).
… On 13 May 2019, at 15:50, Victor Sigler ***@***.***> wrote:
Hey, @edwinveger I was checking out your PR and I figured out what was wrong with it in Linux! We have some compilation conditions for platforms in that specific part of the code and it's not necessary anymore:
https://github.com/httpswift/swifter/blob/2f60ae4676814706ae02fee225315336a550282c/XCode/Sources/HttpResponse.swift#L34-L53
The condition is not longer necessary as we can use JSONSerialization in Linux now. So replacing it with:
case .json(let object):
guard JSONSerialization.isValidJSONObject(object) else {
throw SerializationError.invalidObject
}
let data = try JSONSerialization.data(withJSONObject: object)
return (data.count, {
try $0.write(data)
})
Would make your tests pass on macOS and Linux 😄. Can you please update your PR to reflect this change ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Update changelog Include the new tests added to the `XCTManifests.swift`
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.
This looks good! Thanks for your contribution @edwinveger.
…y-json-dictionary-literals Change HttpResponseBody.json to take Any
This PR addresses issue #392 . I have added several unit tests for this behavior.