Skip to content
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

Add json request support in router #329

Merged
merged 14 commits into from
Apr 23, 2018
Merged

Add json request support in router #329

merged 14 commits into from
Apr 23, 2018

Conversation

matteocrippa
Copy link

@matteocrippa matteocrippa commented Apr 3, 2018

I added body access to allow this

@matteocrippa matteocrippa changed the title Add json request support Add json request support in router Apr 3, 2018
@matteocrippa
Copy link
Author

matteocrippa commented Apr 3, 2018

@kittinunf is it possible that tests for RequestTest.kt are failing?

Seems:

 timeoutInMillisecond = 15000,
 timeoutReadInMillisecond = 15000

were missing

@kittinunf
Copy link
Owner

Yes, you are right. I am fixing the test in this PR #334. After that was merged, can you please rebase against new master?

@matteocrippa
Copy link
Author

@kittinunf ok, ping me if you notice that I'm missing the merge :)

@kittinunf
Copy link
Owner

kittinunf commented Apr 18, 2018

#334 It got merged. Can you rebase? And I will be happy to see this PR get merged 👍 🙏

Ah, sorry. You seem to did that already. Ah you haven't yet.

@@ -15,6 +15,7 @@ targetCompatibility = JavaVersion.VERSION_1_6
dependencies {
compile "org.jetbrains.kotlin:kotlin-stdlib:$kotlinVersion"
compile "com.github.kittinunf.result:result:$resultVersion"
compile 'org.json:json:20170516'
Copy link
Owner

@kittinunf kittinunf Apr 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, do we use this somewhere in the core?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, we need it for tests to manipulate and create a json, dunno why I was not able to get any other access to JSON

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For tests? Then, perhaps using testCompile is better?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need that to create a JSON object to test sending that as body

val json = JSONObject()
                         json.put("id", this.value)
                         json.toString()

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, here right?

  is GetParamsTest -> {
    val json = JSONObject()
    json.put("id", this.value)
    json.toString()

But it is only in the test right? So you can technically just do testCompile 'org.json:json:20170516' so that it doesn't forcefully push dependencies to the users.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, fixed switching it to testCompile

@matteocrippa
Copy link
Author

@kittinunf it seems it is now the Jackson test failing

com.github.kittinunf.fuel.FuelJacksonTest > jacksonTestResponseSyncObject FAILED
    com.github.kittinunf.fuel.core.FuelError at FuelJacksonTest.kt:190
        Caused by: com.github.kittinunf.fuel.core.HttpException at FuelJacksonTest.kt:190
com.github.kittinunf.fuel.FuelJacksonTest > jacksonTestResponseSyncObjectError FAILED
    java.lang.AssertionError at FuelJacksonTest.kt:119

@codecov
Copy link

codecov bot commented Apr 18, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@56cf7ce). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #329   +/-   ##
=========================================
  Coverage          ?   77.21%           
  Complexity        ?      193           
=========================================
  Files             ?       32           
  Lines             ?      904           
  Branches          ?      148           
=========================================
  Hits              ?      698           
  Misses            ?      131           
  Partials          ?       75
Impacted Files Coverage Δ Complexity Δ
...tlin/com/github/kittinunf/fuel/util/FuelRouting.kt 100% <100%> (ø) 0 <0> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56cf7ce...d0686bb. Read the comment docs.

@matteocrippa
Copy link
Author

Ok, now we should have all the stuff in the right place for testing

@kittinunf
Copy link
Owner

Awesome, merging this in.

@kittinunf kittinunf merged commit 1a1891c into kittinunf:master Apr 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants