Add constant OkHttp.VERSION

[Kevinrob]

Related to issue #5969

Since OkHttp 4.0.0, the okhttp3.internal package is not a published API.
It's impossible to get default userAgent without access to this.

As @yschimke suggest, we can move Version from package okhttp3.internal to okhttp3.

[yschimke]

As @yschimke suggest, we can move Version from package okhttp3.internal to okhttp3.

By "Java package implementation version" I meant whether the right fix is to make sure this works

    OkHttpClient::class.java.`package`.implementationVersion

And not have a new Version class

[Kevinrob]

As @yschimke suggest, we can move Version from package okhttp3.internal to okhttp3.

By "Java package implementation version" I meant whether the right fix is to make sure this works

    OkHttpClient::class.java.`package`.implementationVersion

And not have a new Version class

I'm not sure to understand 😅
Here, I just changed the Java package of the class Version. I'm not adding a new Version class 🙂

[yschimke]

Yep, rather than new API, maybe it's better to expose the version number using existing mechanics.

https://docs.oracle.com/javase/tutorial/deployment/jar/packageman.html

Anyway, I'll let others weigh in. I wouldn't change yet. The danger of this new API is it encourages people to use it, which we may not want to encourage.

[Kevinrob]

Yep, rather than new API, maybe it's better to expose the version number using existing mechanics.

https://docs.oracle.com/javase/tutorial/deployment/jar/packageman.html

Anyway, I'll let others weigh in. I wouldn't change yet. The danger of this new API is it encourages people to use it, which we may not want to encourage.

Okay, I see 👍
But I think that it's a good thing to be able to get default userAgent. If someone need to change it and going back to default for example.

[yschimke]

Was hoping it was this simple in the build.gradle for okhttp jar, but not working.

jar {
  manifest {
    attributes('Automatic-Module-Name': 'okhttp3', 'Implementation-Version': project.version)
  }
}

So maybe this PR is the right approach?

[yschimke]

I'm not sold on just exposing this previously internal method as public API. For example the Conscrypt version as a public API is usable. https://github.com/google/conscrypt/blob/ce2fbf2d740437655c7774c4c0cd3c3f8ee159f9/common/src/main/java/org/conscrypt/Conscrypt.java#L58

    public static class Version {
        private final int major;
        private final int minor;
        private final int patch;

        private Version(int major, int minor, int patch) {
            this.major = major;
            this.minor = minor;
            this.patch = patch;
        }

        public int major() { return major; }
        public int minor() { return minor; }
        public int patch() { return patch; }
    }

    private static final Version VERSION;

    static {
        int major = -1;
        int minor = -1;
        int patch = -1;
        InputStream stream = null;
        try {
            stream = Conscrypt.class.getResourceAsStream("conscrypt.properties");
            if (stream != null) {
                Properties props = new Properties();
                props.load(stream);
                major = Integer.parseInt(props.getProperty("org.conscrypt.version.major", "-1"));
                minor = Integer.parseInt(props.getProperty("org.conscrypt.version.minor", "-1"));
                patch = Integer.parseInt(props.getProperty("org.conscrypt.version.patch", "-1"));
            }
        } catch (IOException e) {
        } finally {
            IoUtils.closeQuietly(stream);
        }
        if ((major >= 0) && (minor >= 0) && (patch >= 0)) {
            VERSION = new Version(major, minor, patch);
        } else {
            VERSION = null;
        }
    }

For now, I think building a properties file or including in the Jar manifest, is better than just uplifting this API.

n.b. I think JAR versioning is broken in Android

Conscrypt::class.java.`package`?.implementationVersion
# 0.0

[swankjesse]

Shall we create a version constant? We can say that we do semver and sufficiently motivated callers can use the Maven rules to parse it.

OkHttp.VERSION

[yschimke]

Shall we create a version constant? We can say that we do semver and sufficiently motivated callers can use the Maven rules to parse it.

OkHttp.VERSION

Are there any subtleties to that? If we just change the @ filename annotation in a Version.kt file, does that grab this top level file class forever?

[swankjesse]

Or maybe the file name is OkHttp.kt and it's a single const field.

My preference is we document the version being strictly confirmant to semver 2.0.0. We will need to be careful to avoid version names where Maven and Semver disagree.

[swankjesse]

For anyone following along curious about where Maven and Semver disagree on order:
https://publicobject.com/2019/12/18/naming-versions/

[yschimke]

@Kevinrob Thanks for your patience on this, does this version field suit your needs? Care to update the PR?

[Kevinrob]

@Kevinrob Thanks for your patience on this, does this version field suit your needs? Care to update the PR?

@yschimke Yes. I updated the PR with @swankjesse proposition 👍

[yschimke]

@yschimke Yes. I updated the PR with @swankjesse proposition 👍

Looks good, I think it's worth a test because of the build dependency. Would you add a single test for this, starts with "4." Or whatever you think makes sense.

[yschimke]

* What went wrong:

Execution failed for task ':okhttp:spotlessKotlin'.

> The following files had format violations:
      okhttp/src/test/java/okhttp3/OkHttpTest.kt
          @@ -8,4 +8,4 @@
           ········val·semVerRegex·=·Regex("^[0-9]+\\.[0-9]+\\.[0-9]+(-.+)?$")
           ········assert(semVerRegex.matches(VERSION))
           ····}
          -}
          +}
  Run 'gradlew spotlessApply' to fix these violations.

[yschimke]

instead of java assertions, can you replace with assertj?

import org.assertj.core.api.Assertions.assertThat
...
assertThat(VERSION).matches("[0-9]+\\.[0-9]+\\.[0-9]+(-.+)?")

[Kevinrob]

Yes, changed ✔

[yschimke]

@Kevinrob sorry for being finicky, and thanks for working through this.

[Kevinrob]

sorry for being finicky, and thanks for working through this.

@yschimke No problem 🙂

[Kevinrob]

Thank you @yschimke for merging this PR!
Is OkHttp project accept contribution about compilation warning correction?

[yschimke]

If it's a clear improvement to the code, but not if it's just to appease a lint rule/warning that we don't generally agree with. We don't actively sprinkle code with annotations to appease Intellij.