Move array population to single iterator code path #623

bitwiseman · 2019-11-24T02:02:44Z

This removes a second code path through pagination.

This remove a second code path throught pagination.

bitwiseman · 2019-12-13T20:29:14Z

@PauloMigAlmeida
Any thoughts on this? I don't have a lot of folks to ask for code review, so you get the honors. 😄

bitwiseman · 2019-12-13T20:30:48Z

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

@@ -425,7 +425,7 @@ URL getApiURL(String tailApiUrl) throws IOException {
    }

    Requester retrieve() {
-        return new Requester(this).method("GET");
+        return new Requester(this);


Requester defaults to GET now.

bitwiseman · 2019-12-13T20:31:18Z

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

 import java.util.function.Consumer;
 import java.util.logging.Logger;
-import java.util.regex.Matcher;


No more regex-ing needed.

bitwiseman · 2019-12-13T20:32:12Z

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

-        for (Entry e : args) {
-            if (e.key.equals(key)) {
-                e.value = value;
+        for (int index = 0; index < args.size(); index++) {


With final Entry fields, we now replace the instance rather than changing the values.

bitwiseman · 2019-12-13T20:34:11Z

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

+     *            the content type
+     * @return the requester
+     */
+    Requester setRawUrlPath(String urlOrPath) {


Eventually, we'll want to call this method directly where it is needed, but for now withUrlPath has logic to decide when to use this.

bitwiseman · 2019-12-13T20:37:47Z

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

+     * @throws IOException
+     *             if the server returns 4xx/5xx responses.
+     */
+    public <T> T[] toArray(@Nonnull Class<T[]> type) throws IOException {


Overloading to() doesn't work with T[] and T. The compiler can't handle it. Instead moved this logic to this new method.

This results in cleaner and clearer code - there is no case where the caller doesn't already know whether they want an array or an instance returned so why call one method and then immediately have an if statement.

bitwiseman · 2019-12-13T20:41:14Z

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

            try {
-                T result = parse(type, instance);
-                if (type != null && type.isArray()) { // we might have to loop for pagination - done through recursion
-                    final String links = uc.getHeaderField("link");


This same work as done by the PagingIterator<T> - see toArray().

bitwiseman · 2019-12-13T20:44:30Z

...rces/org/kohsuke/github/AppTest/wiremock/notifications/mappings/notifications-10-a97934.json

@@ -2,7 +2,7 @@
  "id": "a979348d-c6be-4cb7-8877-7c42a6f013ae",
  "name": "notifications",
  "request": {
-    "url": "/notifications?all=true&page=9&all=true",


And by switching to one consistent path we find little bugs like this. Yay!

PauloMigAlmeida · 2019-12-13T23:15:09Z

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

@@ -23,7 +23,7 @@
    GHAppCreateTokenBuilder(GitHub root, String apiUrlTail, Map<String, GHPermissionType> permissions) {
        this.root = root;
        this.apiUrlTail = apiUrlTail;
-        this.builder = new Requester(root);
+        this.builder = root.retrieve().method("POST");


Shouldn't that be just root.retrieve() as we are setting the method on the create() method?

PauloMigAlmeida · 2019-12-13T23:17:32Z

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

@@ -135,7 +135,7 @@ public String getBrowserDownloadUrl() {
    }

    private void edit(String key, Object value) throws IOException {
-        new Requester(root).with(key, value).method("PATCH").to(getApiRoute());
+        root.retrieve().method("POST").with(key, value).method("PATCH").withUrlPath(getApiRoute()).to();


Maybe remove the method("POST") as we are setting method("PATCH") almost right after it

PauloMigAlmeida · 2019-12-13T23:18:31Z

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

@@ -13,7 +13,7 @@

    GHBlobBuilder(GHRepository repo) {
        this.repo = repo;
-        req = new Requester(repo.root);
+        req = repo.root.retrieve().method("POST");


PauloMigAlmeida · 2019-12-13T23:22:33Z

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

@@ -123,7 +123,7 @@ GHBranchProtection wrap(GHBranch branch) {
    }

    private Requester requester() {
-        return new Requester(root).withPreview(ZZZAX);
+        return root.retrieve().method("POST").withPreview(ZZZAX);


Maybe replace this with return root.retrieve()..withPreview(ZZZAX);

PauloMigAlmeida · 2019-12-13T23:23:24Z

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

@@ -87,7 +87,7 @@ public RequiredReviews getRequiredReviews() {
    @Preview
    @Deprecated
    public boolean getRequiredSignatures() throws IOException {
-        return requester().method("GET").to(url + REQUIRE_SIGNATURES_URI, RequiredSignatures.class).enabled;
+        return requester().method("GET").withUrlPath(url + REQUIRE_SIGNATURES_URI).to(RequiredSignatures.class).enabled;


If you implement the suggestion of the private Requester requester() then you can get rid of .method("GET") and take advantage of the default HTTP method

PauloMigAlmeida · 2019-12-14T00:05:50Z

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

@@ -176,10 +176,13 @@ public void markAsRead() throws IOException {
     *             the io exception
     */
    public GHSubscription subscribe(boolean subscribed, boolean ignored) throws IOException {
-        return new Requester(root).with("subscribed", subscribed)
+        return root.retrieve()
+                .method("POST")


redundant method("POST")

PauloMigAlmeida · 2019-12-14T00:06:52Z

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

@@ -33,7 +33,7 @@ private TreeEntry(String path, String mode, String type) {

    GHTreeBuilder(GHRepository repo) {
        this.repo = repo;
-        req = new Requester(repo.root);
+        req = repo.root.retrieve().method("POST");


You could remove method("POST") as you set the HTTP method on create

PauloMigAlmeida · 2019-12-14T01:55:15Z

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

+
+                for (Iterator<Entry> it = args.listIterator(); it.hasNext();) {
+                    Entry arg = it.next();
+                    argString.append(URLEncoder.encode(arg.key, "UTF-8"));


Maybe using this one https://docs.oracle.com/javase/7/docs/api/java/nio/charset/StandardCharsets.html#UTF_8 instead of the hardcoded value? (no biggie tbh)

PauloMigAlmeida · 2019-12-14T01:59:14Z

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

-                throw new AssertionError(e); // UTF-8 is mandatory
-            }
-        }
+        String tailApiUrl = buildTailApiUrl(urlPath);


PauloMigAlmeida · 2019-12-14T02:01:19Z

...rces/org/kohsuke/github/AppTest/wiremock/notifications/mappings/notifications-10-a97934.json

@@ -2,7 +2,7 @@
  "id": "a979348d-c6be-4cb7-8877-7c42a6f013ae",
  "name": "notifications",
  "request": {
-    "url": "/notifications?all=true&page=9&all=true",


PauloMigAlmeida · 2019-12-14T02:09:54Z

@PauloMigAlmeida
Any thoughts on this? I don't have a lot of folks to ask for code review, so you get the honors. 😄

My pleasure ;)

@bitwiseman looks great! This should open the possibilities for a lot of improvements that we always wanted to do 🍾

The only thing which I still struggling is the name of the to method. Now that is doesn't take the URL path anymore, it sounds a bit funny. For instance:

GHHook hook = root.retrieve()
                  .method("POST")
                  .with("name", name)
                  .with("active", active)
                  .with("config", config)
                  .with("events", ea)
                  .withUrlPath(collection())
                  .to(clazz());

In the case above it looks like it's performing a parseTo/fromJson kind of thing, while in this other below:

root.retrieve().method("PATCH").withUrlPath("/user/repository_invitations/" + id).to();

This looks like it's performing a execute/sendRequest.

Like any other developer, I too struggle with coming up with variable/method names 😄 but if you could think of a name for the former to() method that can read nicely in both scenarios then it's perfect 👍

bitwiseman · 2019-12-16T16:51:58Z

@PauloMigAlmeida
Okay, I'll go through and clean up all the redundant method() calls. Thanks!

Naming: Agreed, it could be better. The to() method should really be something like send().

bitwiseman force-pushed the task/iterator branch from 9123e95 to f4bc591 Compare November 26, 2019 02:12

bitwiseman added 2 commits November 25, 2019 18:15

Move array population to single iterator code path

1b55b5f

This remove a second code path throught pagination.

Change requester to GET by default

3296cef

bitwiseman force-pushed the task/iterator branch from f4bc591 to 3296cef Compare November 26, 2019 02:15

bitwiseman added 2 commits November 26, 2019 15:14

Force URI encoding for api paths starting with slash

60700d5

Move url to separate method

0f94828

bitwiseman commented Dec 13, 2019

View reviewed changes

PauloMigAlmeida approved these changes Dec 14, 2019

View reviewed changes

bitwiseman force-pushed the task/iterator branch 2 times, most recently from 6ba9a3d to a6945bb Compare December 17, 2019 17:49

Clean up request method calls

40f05e4

bitwiseman force-pushed the task/iterator branch from a6945bb to 40f05e4 Compare December 17, 2019 18:03

bitwiseman added 3 commits December 17, 2019 12:41

Rename methods for better clarity

305267d

Additional tweaks to reduce the number of code paths

52dd90e

Merge remote-tracking branch 'github-api/master' into task/iterator

049db83

bitwiseman merged commit 6bc617c into hub4j:master Dec 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move array population to single iterator code path #623

Move array population to single iterator code path #623

bitwiseman commented Nov 24, 2019

bitwiseman commented Dec 13, 2019

bitwiseman Dec 13, 2019

bitwiseman Dec 13, 2019

bitwiseman Dec 13, 2019

bitwiseman Dec 13, 2019

bitwiseman Dec 13, 2019

bitwiseman Dec 13, 2019 •

edited

Loading

bitwiseman Dec 13, 2019

PauloMigAlmeida Dec 14, 2019

PauloMigAlmeida Dec 13, 2019

PauloMigAlmeida Dec 13, 2019

PauloMigAlmeida Dec 13, 2019

PauloMigAlmeida Dec 13, 2019

PauloMigAlmeida Dec 13, 2019

PauloMigAlmeida Dec 14, 2019

PauloMigAlmeida Dec 14, 2019

PauloMigAlmeida Dec 14, 2019

PauloMigAlmeida Dec 14, 2019

PauloMigAlmeida Dec 14, 2019

PauloMigAlmeida commented Dec 14, 2019

bitwiseman commented Dec 16, 2019

Move array population to single iterator code path #623

Move array population to single iterator code path #623

Conversation

bitwiseman commented Nov 24, 2019

bitwiseman commented Dec 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitwiseman Dec 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PauloMigAlmeida commented Dec 14, 2019

bitwiseman commented Dec 16, 2019

bitwiseman Dec 13, 2019 •

edited

Loading