-
Notifications
You must be signed in to change notification settings - Fork 774
Add Builder/Creator/Updater for GHLabel #724
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
Merged
Merged
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
2607d6a
Make GHLabel example of proposed API design
bitwiseman b7de435
Alternative proposal
bitwiseman 0cb2371
Third alternative proposal
bitwiseman 134222f
Minor cleanups
bitwiseman 98ef2cc
Update-in-place and safer single or batch calculation
bitwiseman f37e4bd
Private fields
bitwiseman 64a82f4
Clean up proposed API changes
bitwiseman 9f1d732
Reverted getter changes to highlight the more important set/update ch…
bitwiseman 79a1bb3
Update src/test/java/org/kohsuke/github/AppTest.java
bitwiseman 863ad0f
Clarify behavior
bitwiseman 09ec89b
Remove Repository member from GHLabel
bitwiseman fcb8d03
Ensure that Description is part of GHLabel comparision
bitwiseman b818031
Change to Preview for new builder pattern
bitwiseman b15e0d4
Cleanup and tweaks
bitwiseman 2ab4eaf
Tweaks and cleanup
bitwiseman 772272f
Re-record test for GHLabel
bitwiseman File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,165 @@ | ||
| package org.kohsuke.github; | ||
|
|
||
| import java.io.IOException; | ||
|
|
||
| import javax.annotation.CheckForNull; | ||
| import javax.annotation.Nonnull; | ||
|
|
||
| /** | ||
| * An abstract data object builder/updater. | ||
| * | ||
| * This class can be use to make a Builder that supports both batch and single property changes. | ||
| * <p> | ||
| * Batching looks like this: | ||
| * | ||
| * <pre> | ||
| * update().someName(value).otherName(value).done() | ||
| * </pre> | ||
| * </p> | ||
| * <p> | ||
| * Single changes look like this: | ||
| * | ||
| * <pre> | ||
| * set().someName(value); | ||
| * set().otherName(value); | ||
| * </pre> | ||
| * </p> | ||
| * <p> | ||
| * If {@link S} is the same as {@link R}, {@link #with(String, Object)} will commit changes after the first value change | ||
| * and return a {@link R} from {@link #done()}. | ||
| * </p> | ||
| * <p> | ||
| * If {@link S} is not the same as {@link R}, {@link #with(String, Object)} will batch together multiple changes and let | ||
| * the user call {@link #done()} when they are ready. | ||
| * | ||
| * @param <R> | ||
| * Final return type built by this builder returned when {@link #done()}} is called. | ||
| * @param <S> | ||
| * Intermediate return type for this builder returned by calls to {@link #with(String, Object)}. If {@link S} | ||
| * the same as {@link R}, this builder will commit changes after each call to {@link #with(String, Object)}. | ||
| */ | ||
| abstract class AbstractBuilder<R, S> { | ||
|
|
||
| @Nonnull | ||
| private final Class<R> returnType; | ||
|
|
||
| private final boolean commitChangesImmediately; | ||
|
|
||
| @CheckForNull | ||
| private final R baseInstance; | ||
|
|
||
| @Nonnull | ||
| protected final Requester requester; | ||
|
|
||
| // TODO: Not sure how update-in-place behavior should be controlled | ||
| // However, it certainly can be controlled dynamically down to the instance level or inherited for all children of | ||
| // some | ||
| // connection. | ||
| protected boolean updateInPlace; | ||
|
|
||
| /** | ||
| * Creates a builder. | ||
| * | ||
| * @param root | ||
| * the GitHub instance to connect to. | ||
| * @param intermediateReturnType | ||
| * the intermediate return type of type {@link S} returned by calls to {@link #with(String, Object)}. | ||
| * Must either be equal to {@code builtReturnType} or this instance must be castable to this class. If | ||
| * not, the constructor will throw {@link IllegalArgumentException}. | ||
| * @param finalReturnType | ||
| * the final return type for built by this builder returned when {@link #done()}} is called. | ||
| * @param baseInstance | ||
| * optional instance on which to base this builder. | ||
| */ | ||
| protected AbstractBuilder(@Nonnull Class<R> finalReturnType, | ||
| @Nonnull Class<S> intermediateReturnType, | ||
| @Nonnull GitHub root, | ||
| @CheckForNull R baseInstance) { | ||
| this.requester = root.createRequest(); | ||
| this.returnType = finalReturnType; | ||
| this.commitChangesImmediately = returnType.equals(intermediateReturnType); | ||
| if (!commitChangesImmediately && !intermediateReturnType.isInstance(this)) { | ||
| throw new IllegalArgumentException( | ||
| "Argument \"intermediateReturnType\": This instance must be castable to intermediateReturnType or finalReturnType must be equal to intermediateReturnType."); | ||
| } | ||
|
|
||
| this.baseInstance = baseInstance; | ||
| this.updateInPlace = false; | ||
| } | ||
|
|
||
| /** | ||
| * Finishes an update, committing changes. | ||
| * | ||
| * This method may update-in-place or not. Either way it returns the resulting instance. | ||
| * | ||
| * @return an instance with updated current data | ||
| * @throws IOException | ||
| * if there is an I/O Exception | ||
| */ | ||
| @Nonnull | ||
| @Preview | ||
| @Deprecated | ||
| public R done() throws IOException { | ||
| R result; | ||
| if (updateInPlace && baseInstance != null) { | ||
| result = requester.fetchInto(baseInstance); | ||
| } else { | ||
| result = requester.fetch(returnType); | ||
| } | ||
| return result; | ||
| } | ||
|
|
||
| /** | ||
| * Applies a value to a name for this builder. | ||
| * | ||
| * If {@link S} is the same as {@link R}, this method will commit changes after the first value change and return a | ||
| * {@link R} from {@link #done()}. | ||
| * | ||
| * If {@link S} is not the same as {@link R}, this method will return an {@link S} and letting the caller batch | ||
| * together multiple changes and call {@link #done()} when they are ready. | ||
| * | ||
| * @param name | ||
| * the name of the field | ||
| * @param value | ||
| * the value of the field | ||
| * @return either a continuing builder or an updated data record | ||
| * @throws IOException | ||
| * if an I/O error occurs | ||
| */ | ||
| @Nonnull | ||
| @Preview | ||
| @Deprecated | ||
| protected S with(@Nonnull String name, Object value) throws IOException { | ||
| requester.with(name, value); | ||
| return continueOrDone(); | ||
| } | ||
|
|
||
| /** | ||
| * Chooses whether to return a continuing builder or an updated data record | ||
| * | ||
| * If {@link S} is the same as {@link R}, this method will commit changes after the first value change and return a | ||
| * {@link R} from {@link #done()}. | ||
| * | ||
| * If {@link S} is not the same as {@link R}, this method will return an {@link S} and letting the caller batch | ||
| * together multiple changes and call {@link #done()} when they are ready. | ||
| * | ||
| * @return either a continuing builder or an updated data record | ||
| * @throws IOException | ||
| * if an I/O error occurs | ||
| */ | ||
| @Nonnull | ||
| @Preview | ||
| @Deprecated | ||
| protected S continueOrDone() throws IOException { | ||
| // This little bit of roughness in this base class means all inheriting builders get to create Updater and | ||
| // Setter classes from almost identical code. Creator can often be implemented with significant code reuse as | ||
| // well. | ||
| if (commitChangesImmediately) { | ||
| // These casts look strange and risky, but they they're actually guaranteed safe due to the return path | ||
| // being based on the previous comparison of class instances passed to the constructor. | ||
| return (S) done(); | ||
| } else { | ||
| return (S) this; | ||
| } | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BulkChange in core uses
commitandabort. abort is not needed. Maybesendis a good choice. orcall?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The two places this will get used look something like this:
send()could work.callnot as much. I'd just like to make sure we've considered alternatives before releasing this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.