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 a proper Kotlin constructor for Request #7208

Merged
merged 3 commits into from
Apr 4, 2022

Conversation

swankjesse
Copy link
Collaborator

This turns out to be very useful throughout our test suite.

This turns out to be very useful throughout our test suite.
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.fail;

public final class RequestTest {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Converted to Kotlin.

}
}

@Test public void headerForbidsNullArguments() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This particular test dropped.

import org.assertj.core.api.Assertions.fail
import org.junit.jupiter.api.Test

class RequestTest {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Converted from Java. The first 4 tests are new.

@yschimke
Copy link
Collaborator

yschimke commented Apr 3, 2022


RequestTest[jvm] > multipleTags()[jvm] FAILED
    java.lang.AssertionError: 
    Expecting actual:
      20170815L
    and actual:
      20170815L
    to refer to the same object
        at okhttp3.RequestTest.multipleTags(RequestTest.kt:427)

constructor(
url: HttpUrl,
headers: Headers = Headers.headersOf(),
method: String = "\u0000", // Sentinel value chooses based on what the body is.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Cute, was a bit against it at first, but it hits the sweet spot for usage.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It also removes the comment I came here to make, that HttpMethods should be public API, since you need to pass in here. But not an issue since this can be left out mostly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I considered null as the sentinel, but that lets users pass null which I don’t like.

I considered making the when expression the default value, but that puts the parameters in a surprising order.

This is ugly, but users shouldn’t see it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

2028: IETF formalises the \u0000 Verb based on widespread popularity of the just do what you need to semantics...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ooooh think of the poor C programmers, strlen on this guy isn't fun.

Copy link
Collaborator

@yschimke yschimke left a comment

Choose a reason for hiding this comment

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

My concerns with the Constructor were just around additional params, but it seems to satisfy the typical cases, and use the Builder for anything more exotic (tags, ).

@yschimke
Copy link
Collaborator

yschimke commented Apr 3, 2022

RequestTest[jvm] > multipleTags()[jvm] FAILED
    java.lang.AssertionError: 
    Expecting actual:
      20170815L
    and actual:
      20170815L
    to refer to the same object
        at okhttp3.RequestTest.multipleTags(RequestTest.kt:427)
val longTag: Long? = 20170815L

@swankjesse swankjesse merged commit 79f50e1 into master Apr 4, 2022
@oldergod oldergod deleted the jwilson.0403.request_constructor branch April 4, 2022 13:51
Copy link

@KikilLovex7 KikilLovex7 left a comment

Choose a reason for hiding this comment

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

okhttp/src/jvmMain/kotlin/okhttp3/Cache.kt

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.

4 participants