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

HttpUrl in Kotlin #4745

Merged
merged 3 commits into from
Mar 19, 2019
Merged

HttpUrl in Kotlin #4745

merged 3 commits into from
Mar 19, 2019

Conversation

swankjesse
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@JakeWharton JakeWharton left a comment

Choose a reason for hiding this comment

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

Sorry lots of comments...

'okhttp3.HttpUrl$Builder#setEncodedQueryParameter(java.lang.String, java.lang.String)',
'okhttp3.HttpUrl$Builder#setPathSegment(int, java.lang.String)',
'okhttp3.HttpUrl$Builder#setQueryParameter(java.lang.String, java.lang.String)',
'okhttp3.HttpUrl$Builder#username(java.lang.String)',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yikes! This list seems very problematic because it means they're not being verified at all anymore. Do newer versions of japicmp handle this? I filed an issue about updating the Gradle plugin (melix/japicmp-gradle-plugin#25) but I suspect I'll have to do it myself since Cedric is a very busy person.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, this is gross. Need to fix japicmp.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

You da best. Guess it's my turn to send the PR to the Gradle plugin then!

class HttpUrl internal constructor(builder: Builder) {

/** Either "http" or "https". */
internal val scheme: String = builder.scheme ?: throw IllegalStateException("scheme == null")
Copy link
Collaborator

Choose a reason for hiding this comment

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

could also be

internal val scheme: String = checkNotNull(builder.scheme) { "scheme == null" }

no preference myself

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’m gonna keep as-is to keep the exception the same.

val isHttps: Boolean get() = scheme == "https"

/** Returns this URL as a [java.net.URL][URL]. */
fun url(): URL {
Copy link
Collaborator

Choose a reason for hiding this comment

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

idiomatic naming would be toUrl on the Kotlin side

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 want to defer Kotlin-facing API changes until it’s all migrated over. When we do that we’ll need to start writing .kt test cases.

* These differences may have a significant consequence when the URI is interpreted by a
* web server. For this reason the [URI class][URI] and this method should be avoided.
*/
fun uri(): URI {
Copy link
Collaborator

Choose a reason for hiding this comment

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

similarly, toUri

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call. On the backlog.

}

/** Returns either "http" or "https". */
fun scheme(): String = scheme
Copy link
Collaborator

Choose a reason for hiding this comment

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

property?

Copy link
Collaborator

Choose a reason for hiding this comment

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

seems like nearly everything that was a method should be a read-only property or computed property. this is a data class, can we make it one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Heck yes. But for follow-up.

* null if it has any other protocol.
*/
@JvmStatic
operator fun get(url: URL): HttpUrl? = parse(url.toString())
Copy link
Collaborator

Choose a reason for hiding this comment

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

URL.toHttpUrlOrNull()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For later!

operator fun get(url: URL): HttpUrl? = parse(url.toString())

@JvmStatic
operator fun get(uri: URI): HttpUrl? = parse(uri.toString())
Copy link
Collaborator

Choose a reason for hiding this comment

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

URI.toHttpUrlOrNull()

var codePoint: Int
var i = pos
while (i < limit) {
codePoint = input.codePointAt(i)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Kotlin needs String.forEachCodepoint

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure does. We should build an internal inline function for this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

private val encodedValues: List<String> = Util.immutableList(encodedValues)

/** The number of key-value pairs in this form-encoded body. */
fun size(): Int = encodedNames.size
Copy link
Collaborator

Choose a reason for hiding this comment

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

property + @getJvmName("size") seems more idiomatic

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed.


var i = 0
val size = encodedNames.size
while (i < size) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

for (i in 0 until encodedNames.size)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

okhttp/src/main/java/okhttp3/FormBody.java Outdated Show resolved Hide resolved

/** Returns this URL as a {@link URL java.net.URL}. */
public URL url() {
val isHttps: Boolean get() = scheme == "https"
Copy link
Collaborator

Choose a reason for hiding this comment

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

scheme is a val, so this can just be a precomputed property without the get()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

* <tr><td>{@code http://username@host/}</td><td>{@code "username"}</td></tr>
* <tr><td>{@code http://username:password@host/}</td><td>{@code "username"}</td></tr>
* <tr><td>{@code http://a%20b:c%20d@host/}</td><td>{@code "a%20b"}</td></tr>
* <table summary="">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be nice to convert those into Markdown tables, follow-up

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea.

if (queryNamesAndValues == null) return null // No query.
val result = StringBuilder()
namesAndValuesToQueryString(result, queryNamesAndValues)
return result.toString()
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit:

return buildString {
  namesAndValuesToQueryString(this, queryNamesAndValues)
}

/**
* Returns a new {@code HttpUrl} representing {@code url}.
*
* @throws IllegalArgumentException If
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just read about a neat "non-nullable shadow" pattern, you just do

val encodedQueryNamesAndValues = encodedQueryNamesAndValues ?: throw NullPointerException()

and the rest of the code in the method doesn't need the double bangs

/**
* Returns a new {@code HttpUrl} representing {@code url}.
*
* @throws IllegalArgumentException If
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use buildString

if (limit - pos < 2) return -1

val c0 = input[pos]
if ((c0 < 'a' || c0 > 'z') && (c0 < 'A' || c0 > 'Z')) return -1 // Not a scheme start char.
Copy link
Collaborator

Choose a reason for hiding this comment

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

if (c0 !in 'a'..'z' && c0 !in 'A'..'Z')

for (i in pos + 1 until limit) {
val c = input[i]

return if (c >= 'a' && c <= 'z'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same, use ranges for a..z, A..Z, 0..9

try {
// Canonicalize the port string to skip '\n' etc.
val portString = canonicalize(input, pos, limit, "", false, false, false, true, null)
val i = Integer.parseInt(portString)
Copy link
Collaborator

Choose a reason for hiding this comment

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

portString.toInt()

@swankjesse swankjesse merged commit 98a67d1 into master Mar 19, 2019
@oldergod oldergod deleted the jwilson.0318.httpurl branch March 19, 2019 20:58
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