Add Builder/Creator/Updater for GHLabel #724

bitwiseman · 2020-03-02T09:49:21Z

Here's another possible way to address #589 . This PR uses GHLabel as an example.

@rmetzger @halkeye @PauloMigAlmeida @recena @res0nance - There's still a bunch to be done here (site currently fails due to missing JavaDocs for example), but if you look at the changes to the tests you can see what this new syntax will look like. My intent is to create a pattern that can be used consistently across the project. What do you think of this design?

Highlights:

Support for batching changes. Batching looks like this:

label.update().someName(value).otherName(value).done()

Support for single changes. They look like this:

label.set().someName(value); 
label.set().otherName(value)

Support for dynamically controlled "update-in-place" vs "update-to-new-instance" behavior (not active or tested yet). As in Add ability to update Github labels #649, there are times people want/expect update-in-place and other times they don't. This design will allow for both, though for now it only does "update-to-new-instance".
Significant code reuse - the real win of this POC is that these differing scenarios do not each require wildly differing code paths. Batch/single updating is provided by almost identical code paths. In the case of GHLabel, new instance creation does too (though in other classes creation may diverge more). "update-in-place" vs "update-to-new-instance" behavior actually identical except for one if-else in the the base class. This means that we can get this added functionality without significant added complexity.

Technical notes:

Tried to add final to guarantee immutability but decided against it - While guaranteeing immutability via final fields sounds great, it results in a lot of extra code to make it work with Jackson in a way that the compiler doesn't choke on.

bitwiseman · 2020-03-02T10:17:50Z

src/test/java/org/kohsuke/github/AppTest.java


            t = r.getLabel("test");
-            t.setDescription("this is also a test");
+            GHLabel t3 = t.set().description("this is also a test");


Single Update

bitwiseman · 2020-03-02T10:17:58Z

src/test/java/org/kohsuke/github/AppTest.java

+
+            // update works on multiple changes in one call
+            // t.setColor("");
+            t2 = t.update().color("000000").description("It is dark!").done();


Batch update

bitwiseman · 2020-03-02T10:18:57Z

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

-        return root.createRequest()
-                .withUrlPath(getApiTailUrl("labels"))
-                .toIterable(GHLabel[].class, item -> item.wrapUp(this));
+        return GHLabel.readAll(this);


Moved all the calls that create instances of GHLabel into that class.

src/test/java/org/kohsuke/github/AppTest.java

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

res0nance · 2020-03-03T11:12:58Z

Overall I like the idea because of the ability to batch changes together. The api looks very intuitive. The base class is a little confusing for people intending to create a new API but it looks as simple as it can be while providing the benefit of reusability across the project.

bitwiseman · 2020-03-04T06:13:05Z

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

        return description;
    }

-    GHLabel wrapUp(GHRepository repo) {


Replaced wrapUp() with lateBind().
Update: removed need for late bind.

bitwiseman · 2020-03-04T06:16:22Z

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

+     *             if there is an I/O Exception
+     */
+    @Nonnull
+    public R done() throws IOException {


Any suggestions for a better method name than done()? send()? submit()?
We can't use commit() because it will almost surely collide with a git "commit" at least conceptually if not literally.

BulkChange in core uses commit and abort. abort is not needed. Maybe send is a good choice. or call?

The two places this will get used look something like this:

GHLabel label = GHLabel.create().name(name).color(color).description(description).xxxx(); label.update().color(color2).description(description2).xxxx();

send() could work. call not as much. I'd just like to make sure we've considered alternatives before releasing this.

Okay, well, this api is internal, so changing it later is not a breaking API change. I'll go with the current verbs for now and we can change them later if needed.

Hm, doh, no, what was I thinking this is public. Ah, but I can use the same structure we already have for API Previews and ship this now. 😄There we go.

The guts of this version are a bit ugly but they result reasonable API code without a ton of extra code needed.

This removes the from the fields. Functionally the behavior is unchanged but it is no longer guaranteed at compile time. This simplifies streamlines the code slightly, but at the cost of only being able to assert immutability rather than know it. However, as we move to using this structure through more of the library, this is may be a better choice. There are so many places where the GitHub API itself returns partial records or updates them dynamically. Trying to claim immutability where it doesn't exist is not great either.

…anges

It turns out GHLabel instances do not need a reference to their repo, just to root.

#724 broke the site goal from .github/PULL_REQUEST_TEMPLATE.md

bitwiseman commented Mar 2, 2020

View reviewed changes

src/test/java/org/kohsuke/github/AppTest.java Outdated Show resolved Hide resolved

res0nance reviewed Mar 3, 2020

View reviewed changes

src/main/java/org/kohsuke/github/AbstractBuilder.java Outdated Show resolved Hide resolved

bitwiseman force-pushed the task/builder-updater branch from 6ea9e4c to 9111081 Compare March 4, 2020 06:10

bitwiseman commented Mar 4, 2020

View reviewed changes

bitwiseman force-pushed the task/builder-updater branch 3 times, most recently from 53e9da9 to 0b46ce0 Compare March 4, 2020 08:30

bitwiseman mentioned this pull request Mar 4, 2020

Add ability to update Github labels #649

Closed

4 tasks

bitwiseman force-pushed the task/builder-updater branch from 5d4da2e to 208ffac Compare March 4, 2020 21:38

XiongKezhi mentioned this pull request Mar 6, 2020

Add support for the Checks API #520

Closed

bitwiseman marked this pull request as ready for review March 20, 2020 18:44

bitwiseman changed the title ~~Proof of concept proposal for Builder/Creator/Updater design~~ Add Builder/Creator/Updater for GHLabel Mar 20, 2020

bitwiseman force-pushed the task/builder-updater branch from 7c1be13 to fb50ade Compare March 23, 2020 22:39

bitwiseman added 12 commits March 23, 2020 17:40

Make GHLabel example of proposed API design

2607d6a

Alternative proposal

b7de435

The guts of this version are a bit ugly but they result reasonable API code without a ton of extra code needed.

Minor cleanups

134222f

Update-in-place and safer single or batch calculation

98ef2cc

Private fields

f37e4bd

Clean up proposed API changes

64a82f4

Reverted getter changes to highlight the more important set/update ch…

9f1d732

…anges

Update src/test/java/org/kohsuke/github/AppTest.java

79a1bb3

Clarify behavior

863ad0f

Remove Repository member from GHLabel

09ec89b

It turns out GHLabel instances do not need a reference to their repo, just to root.

Ensure that Description is part of GHLabel comparision

fcb8d03

bitwiseman added 3 commits March 23, 2020 17:40

Change to Preview for new builder pattern

b818031

Cleanup and tweaks

b15e0d4

Tweaks and cleanup

2ab4eaf

bitwiseman force-pushed the task/builder-updater branch 4 times, most recently from 29019d4 to b823d68 Compare March 24, 2020 18:11

Re-record test for GHLabel

772272f

bitwiseman force-pushed the task/builder-updater branch from b823d68 to 772272f Compare March 24, 2020 19:32

jglick mentioned this pull request Mar 25, 2020

GHRepository.createCheckRun #753

Merged

4 tasks

bitwiseman merged commit 7a65013 into hub4j:master Mar 25, 2020

bitwiseman deleted the task/builder-updater branch March 25, 2020 18:56

jglick added a commit to jglick/github-api that referenced this pull request Mar 26, 2020

hub4j#724 broke the site goal from .github/PULL_REQUEST_TEMPLATE.md

9c7479f

bitwiseman added a commit that referenced this pull request Mar 26, 2020

Merge pull request #755 from jglick/javadoc

e911e86

#724 broke the site goal from .github/PULL_REQUEST_TEMPLATE.md

bitwiseman mentioned this pull request Apr 1, 2020

Add new command of 'add github labels to REPO' to bot (WIP) jenkins-infra/ircbot#72

Closed

bitwiseman mentioned this pull request May 29, 2020

Add support for Github Discussions API #828

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Builder/Creator/Updater for GHLabel #724

Add Builder/Creator/Updater for GHLabel #724

Uh oh!

bitwiseman commented Mar 2, 2020 •

edited

Loading

Uh oh!

bitwiseman Mar 2, 2020

Uh oh!

bitwiseman Mar 2, 2020

Uh oh!

bitwiseman Mar 2, 2020

Uh oh!

Uh oh!

Uh oh!

res0nance commented Mar 3, 2020

Uh oh!

bitwiseman Mar 4, 2020 •

edited

Loading

Uh oh!

bitwiseman Mar 4, 2020

Uh oh!

res0nance Mar 4, 2020

Uh oh!

bitwiseman Mar 4, 2020

Uh oh!

bitwiseman Mar 17, 2020

Uh oh!

bitwiseman Mar 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Add Builder/Creator/Updater for GHLabel #724

Add Builder/Creator/Updater for GHLabel #724

Uh oh!

Conversation

bitwiseman commented Mar 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

res0nance commented Mar 3, 2020

Uh oh!

bitwiseman Mar 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bitwiseman commented Mar 2, 2020 •

edited

Loading

bitwiseman Mar 4, 2020 •

edited

Loading