Swtich from HttpConnector to GitHubConnector #1290

bitwiseman · 2021-11-08T03:48:05Z

Description

This change has the same aim as #1287 - remove this library's underlying dependency on HttpURLConnection and the limitations it entails.

This PR introduces a new interface called GitHubConnector that replaces HttpConnector. It too has a single method, but this one takes ConnectorRequest and returns ConnectorResponse, interfaces controlled by this library instead of classes tied to any other library or java version.

Significant effort was made to not change user-visible default behavior with this change. Only users who change their code to use a non-adapter implementation of GitHubConnector (such as OkHttpGitHubConnetor) should see any noticeable change and even that should be limited to less used methods such as GitHub.getConnector().

Before submitting a PR:

We love getting PRs, but we hate asking people for the same basic changes every time.

Push your changes to a branch other than main. Create your PR from that branch.
Add JavaDocs and other comments
Write tests that run and pass in CI. See CONTRIBUTING.md for details on how to capture snapshot data.
Run mvn -D enable-ci clean install site locally. If this command doesn't succeed, your change will not pass CI.

When creating a PR:

Fill in the "Description" above.
Enable "Allow edits from maintainers".

codecov · 2021-11-08T04:39:25Z

Codecov Report

Merging #1290 (056d1f0) into main (0601578) will increase coverage by 0.37%.
The diff coverage is 89.74%.

@@             Coverage Diff              @@
##               main    #1290      +/-   ##
============================================
+ Coverage     77.50%   77.88%   +0.37%     
- Complexity     1906     1998      +92     
============================================
  Files           189      194       +5     
  Lines          6002     6209     +207     
  Branches        329      361      +32     
============================================
+ Hits           4652     4836     +184     
- Misses         1150     1159       +9     
- Partials        200      214      +14

Impacted Files	Coverage Δ
src/main/java/org/kohsuke/github/GHCompare.java	`91.93% <ø> (ø)`
...rc/main/java/org/kohsuke/github/HttpConnector.java	`100.00% <ø> (ø)`
...kohsuke/github/extras/okhttp3/OkHttpConnector.java	`93.75% <ø> (ø)`
.../internal/GitHubConnectorHttpConnectorAdapter.java	`79.00% <79.00%> (ø)`
...ation/OrgAppInstallationAuthorizationProvider.java	`93.33% <80.00%> (ø)`
...e/github/extras/okhttp3/OkHttpGitHubConnector.java	`83.82% <83.82%> (ø)`
...ain/java/org/kohsuke/github/AbuseLimitHandler.java	`75.00% <85.71%> (+5.76%)`	⬆️
...main/java/org/kohsuke/github/RateLimitHandler.java	`75.00% <85.71%> (+5.76%)`	⬆️
src/main/java/org/kohsuke/github/GitHubClient.java	`90.33% <92.77%> (+0.86%)`	⬆️
...c/main/java/org/kohsuke/github/GitHubResponse.java	`95.00% <94.11%> (-1.67%)`	⬇️
... and 18 more

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 0601578...056d1f0. Read the comment docs.

nedtwigg

Looks great to me!

nedtwigg · 2021-11-08T04:50:23Z

src/main/java/org/kohsuke/github/RateLimitHandler.java

+     *             the io exception
+     * @see <a href="https://developer.github.com/v3/#rate-limiting">API documentation from GitHub</a>
+     */
+    void onError(GitHubConnectorResponse connectorResponse) throws IOException {


Do you want to @Deprecated the HttpURLConnection method?

src/main/java/org/kohsuke/github/AbuseLimitHandler.java

bitwiseman · 2021-11-08T06:23:23Z

@nedtwigg
I still need to fill in the JavaDoc info.

Any other suggestions?

nedtwigg · 2021-11-08T06:27:18Z

I love when javadoc has example inputs/outputs (this project scores 10/10 there) but I'm annoyed by rigid compliance with a javadoc linter forcing that gold to get diluted with "returns the value" boilerplate. Other than that personal style preference, no comments from me besides merge and release this beauty ;-)

bitwiseman · 2021-11-08T16:42:15Z

I love when javadoc has example inputs/outputs (this project scores 10/10 there) but I'm annoyed by rigid compliance with a javadoc linter forcing that gold to get diluted with "returns the value" boilerplate. Other than that personal style preference, no comments from me besides merge and release this beauty ;-)

Yeah, there is a bunch of autogenerated text to get to compliance. Not great but compliant is still better than non-compliant.

Thanks!

nedtwigg · 2021-11-08T20:28:24Z

Not great but compliant is still better than non-compliant.

Why? I do a lot of work on Spotless, because I think formatting doesn't matter and I don't want to talk about formatting.

When it comes to documentation, I think it does matter, and so it's worth talking about. Automated tools can't say obviously getFoo() returns foo vs it's interesting that getFoo() returns a String guaranteed to start with 'git://'. So whether or not the documentation adds information beyond what's already in the name and type signature of the method is a human judgement call (for now, copilot is coming for us!). IMO, automated enforcement of

does this documentation add information? -> return true, always document everything, users don't need to read the implementation

isn't much better than

does this documentation add information? -> return false, never document anything, users need to read the implementation

If you add options.addStringOption('Xdoclint:none', '-quiet') then it will still error for undefined @link and things like that, but it lets "(to / not to) document" be the judgement call that imo it should be.

</unsolicitedopinion>

bitwiseman · 2021-11-08T22:28:19Z

Re documentation:
I'm all for streamlining where it makes sense.
Hm, could you open an issue for this? I think it is worth discussing on its own. See #727 for similar style discussion that happened a while back.

nedtwigg · 2021-11-08T22:39:51Z

could you open an issue for this

I've thought a lot about this topic, and I don't have any more to add besides this:

Train a low-parameter neural net on the dataset function name+signature -> javadoc for your project.
For each javadoc, diff the neural net's prediction against what the javadoc actually is
Remove all the identical parts of the javadoc

I don't plan to build this, but imo that's the way I would think about it. Happy documenting ;-)

bitwiseman · 2021-11-09T07:54:27Z

src/main/java/org/kohsuke/github/extras/okhttp3/OkHttpConnector.java

@@ -21,12 +32,11 @@
 * @author Liam Newman
 * @author Kohsuke Kawaguchi
 */
-public class OkHttpConnector implements HttpConnector {
+public class OkHttpConnector implements GitHubConnector {


Darn. OkHttpConnector need to continue to use the HttpConnector interface because it is may be extended by clients which will depend on further overriding HttpConnector.connet().

I'll need to create an OkHttpGitHubConnector class that contains this one. I'll also need to retain ObsoleteUrlFactory for now as well. 😢

To preserve backward compatibility, OkHttpConnector must continue to be based on HttpConnector.

This is why we test.

bitwiseman · 2021-11-10T00:18:13Z

Okay, I've got the coverage up and expanded testing to make me confident in this change.

nedtwigg · 2021-11-13T01:48:54Z

Huzzah, thanks a ton! Any blockers to getting this released? I can grab it from JitPack if so.

bitwiseman self-assigned this Nov 8, 2021

bitwiseman added 2 commits November 7, 2021 19:49

Removing depedencyies on HttpUrlConnnection

0a1028c

Convert internals to use GitHubConnector interface

dbd481e

bitwiseman force-pushed the task/response-to-urlconnection branch 4 times, most recently from 8756674 to f1325d2 Compare November 8, 2021 04:34

bitwiseman mentioned this pull request Nov 8, 2021

Introduce ResponseConnector as a superset of HttpConnector #1287

Closed

nedtwigg approved these changes Nov 8, 2021

View reviewed changes

bitwiseman force-pushed the task/response-to-urlconnection branch from 351c16e to a3d183c Compare November 8, 2021 22:59

bitwiseman mentioned this pull request Nov 8, 2021

Consider relaxing the requirements for JavaDoc in the project #1291

Open

bitwiseman force-pushed the task/response-to-urlconnection branch 5 times, most recently from 05087f6 to c2a13c5 Compare November 9, 2021 07:11

bitwiseman commented Nov 9, 2021

View reviewed changes

bitwiseman added 6 commits November 8, 2021 23:59

Implement OkHttpConnector using GitHubConnector

066af55

Set Accept header if it is missing

c2f560a

Fix OkHttpConnnector failures

613e5b0

Add test coverage for OkHttpConnector

3a903a2

Introduce request and response interfaces for GitHubConnector

9db400d

Clean up and code coverage

65363d3

Finish implementation and test of HttpURLConnectionAdapter

e9abaa7

bitwiseman force-pushed the task/response-to-urlconnection branch 2 times, most recently from 1031179 to fc04956 Compare November 9, 2021 08:15

Change OkHttpConnector to OkHttpGitHubConnector

ac9c0ad

To preserve backward compatibility, OkHttpConnector must continue to be based on HttpConnector.

bitwiseman force-pushed the task/response-to-urlconnection branch 7 times, most recently from 41cd58e to cd1faed Compare November 9, 2021 20:42

Further clean up and coverage

55a2b24

This is why we test.

bitwiseman force-pushed the task/response-to-urlconnection branch 3 times, most recently from b8044c5 to e39b3d9 Compare November 9, 2021 23:24

Clean up GitHubConnectorResponseHttpUrlConnectionAdapter

056d1f0

bitwiseman force-pushed the task/response-to-urlconnection branch from e39b3d9 to 056d1f0 Compare November 9, 2021 23:50

bitwiseman merged commit 1535817 into hub4j:main Nov 10, 2021

bitwiseman deleted the task/response-to-urlconnection branch November 10, 2021 00:18

This was referenced Nov 10, 2021

Cleanup GitHub Connection, Client, Request, Response to prepare for making them public #1264

Closed

Add logging of requests and responses #151

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Swtich from HttpConnector to GitHubConnector #1290

Swtich from HttpConnector to GitHubConnector #1290

bitwiseman commented Nov 8, 2021 •

edited

Loading

codecov bot commented Nov 8, 2021 •

edited

Loading

nedtwigg left a comment

nedtwigg Nov 8, 2021

bitwiseman commented Nov 8, 2021

nedtwigg commented Nov 8, 2021

bitwiseman commented Nov 8, 2021

nedtwigg commented Nov 8, 2021

bitwiseman commented Nov 8, 2021

nedtwigg commented Nov 8, 2021

bitwiseman Nov 9, 2021

bitwiseman commented Nov 10, 2021

nedtwigg commented Nov 13, 2021

Swtich from HttpConnector to GitHubConnector #1290

Swtich from HttpConnector to GitHubConnector #1290

Conversation

bitwiseman commented Nov 8, 2021 • edited Loading

Description

Before submitting a PR:

When creating a PR:

codecov bot commented Nov 8, 2021 • edited Loading

Codecov Report

nedtwigg left a comment

Choose a reason for hiding this comment

nedtwigg Nov 8, 2021

Choose a reason for hiding this comment

bitwiseman commented Nov 8, 2021

nedtwigg commented Nov 8, 2021

bitwiseman commented Nov 8, 2021

nedtwigg commented Nov 8, 2021

bitwiseman commented Nov 8, 2021

nedtwigg commented Nov 8, 2021

bitwiseman Nov 9, 2021

Choose a reason for hiding this comment

bitwiseman commented Nov 10, 2021

nedtwigg commented Nov 13, 2021

bitwiseman commented Nov 8, 2021 •

edited

Loading

codecov bot commented Nov 8, 2021 •

edited

Loading