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

Allow dynamic choice of HTTP method #94

Merged
merged 2 commits into from
Dec 10, 2021
Merged

Allow dynamic choice of HTTP method #94

merged 2 commits into from
Dec 10, 2021

Conversation

lihaoyi
Copy link
Member

@lihaoyi lihaoyi commented Dec 10, 2021

Fixes #67
Fixes #52

This was always possible in a roundabout way via

requests.Requester("get", requests.Session()).apply("https://www.google.com")

But this PR adds a convenient alias and documents it for discoverability

requests.send("get")("https://www.google.com")

@lolgab
Copy link
Member

lolgab commented Dec 10, 2021

@lihaoyi By looking at other tests:

diff --git a/requests/test/src/requests/RequestTests.scala b/requests/test/src/requests/RequestTests.scala
index 834eea5..72dd505 100644
--- a/requests/test/src/requests/RequestTests.scala
+++ b/requests/test/src/requests/RequestTests.scala
@@ -82,9 +82,9 @@ object RequestTests extends TestSuite{
 
         val res1 = requests.send("put")(
           "https://httpbin.org/put",
-          data = Map("hello" -> "world", "foo" -> "baz"),
+          data = new RequestBlob.FormEncodedRequestBlob(Map("hello" -> "world", "foo" -> "baz")),
           chunkedUpload = true
-        ).text
+        ).text()
 
         assert(read(res1).obj("form") == Obj("foo" -> "baz", "hello" -> "world"))
       }

Maybe it's some limitation that can be removed, but not in this PR for sure.

@lihaoyi
Copy link
Member Author

lihaoyi commented Dec 10, 2021

@lolgab afaict it's a bug in the scala3 compiler, so I'm fine skipping that test in scala3 for now. The chance of us causing scala3-specific bugs is rather low, and if the bug is on the scala3 side it's their responsibility to fix rather than ours

@lolgab
Copy link
Member

lolgab commented Dec 10, 2021

the .text requiring () is because Scala 3 makes a difference between methods declared like:

def text: String

and if declared like:

def text(): String

as it happens here.

About new FormEncodedRequestBlob() being required, it is probably because Scala 3 doesn't automatically lift implicit parameters into implicit conversions as Scala 2 does.

My 2 cents:
The other tests are written like this and I don't see a reason for send to be special enough to have a src-2 test.
This Scala2/Scala3 source incompatibility should be a open issue instead with the reproduction we want to behave coherently in Scala 2 and Scala 3 and which doesn't today.

@lihaoyi
Copy link
Member Author

lihaoyi commented Dec 10, 2021

.text vs .text() is trivial to fix, but the other is clearly a bug. The fact that the other tests call it with an explicit new RequestBlob.FormEncodedRequestBlob means the other tests are wrong. The correct thing to do is rely on the implicit conversion in Scala2 and disable the tests for Scala3 until we get it working. Nobody is meant to be calling new RequestBlob.FormEncodedRequestBlob in their application code using this library

@lolgab
Copy link
Member

lolgab commented Dec 10, 2021

@lihaoyi Yeah, I get your point!
Let's move this test to src-2 and let's merge it!

@lihaoyi
Copy link
Member Author

lihaoyi commented Dec 10, 2021

I moved all tests involving that implicit conversion to the src-2/ folder, let's see if CI is happy

@lolgab lolgab self-requested a review December 10, 2021 12:14
@lihaoyi lihaoyi merged commit e983d00 into master Dec 10, 2021
@lolgab lolgab deleted the send branch December 10, 2021 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants