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

Introducing ItemGroup.allItems and similar default methods #3148

Merged
merged 2 commits into from Nov 28, 2017

Conversation

4 participants
@jglick
Member

jglick commented Nov 16, 2017

Have noticed (most recently in jenkinsci/active-choices-plugin#17) that it is awkward to call a static utility method on what intuitively should be an instance method, which would be more readable and more discoverable with code completion.

Proposed changelog entries

  • API: Adding some default methods to ItemGroup.

@reviewbybees @jenkinsci/code-reviewers

@jglick jglick requested a review from stephenc Nov 16, 2017

@reviewbybees

This comment has been minimized.

reviewbybees commented Nov 16, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

* and filter them by the given type.
* @since FIXME
*/
default <T extends Item> List<T> getAllItems(Class<T> type) {

This comment has been minimized.

@jglick

jglick Nov 16, 2017

Member

This and others should really be List<? extends T> to signal immutability, but that would be source-incompatible for existing callers via Jenkins.

}
// TODO could delegate to allItems overload taking Authentication, but perhaps more useful to introduce a variant to perform preauth filtering using Predicate and check Item.READ afterwards
// or return a Stream<Item> and provide a Predicate<Item> public static Items.readable(), and see https://stackoverflow.com/q/22694884/12916 if you are looking for just one result

This comment has been minimized.

@jglick

jglick Nov 16, 2017

Member

Could easily write such an impl, but do not have a candidate caller handy to use as a downstream PR, and could find no cases of this idiom in core. (jenkinsci/active-choices-plugin#17 could use it, I was just too lazy to file a PR based on a snapshot baseline for it. Maybe if someone expresses interest.)

@@ -24,6 +25,7 @@
private interface ItemGroupOfNonTopLevelItem extends TopLevelItem, ItemGroup<Item> {}
@Ignore("TODO I am not smart enough to figure out what PowerMock is actually doing; whatever this was testing, better move to the test module and use JenkinsRule")

This comment has been minimized.

@jglick

jglick Nov 16, 2017

Member

Uh, yeah. Apply the transformation in this test suite and both cases fail with an opaque NullPointerException. Leave the suite alone and the other case passes but this one fails with a comparison error—unless you run just this case by itself, in which case it passes.

@jglick jglick added the needs-review label Nov 20, 2017

@oleg-nenashev

🐝 and @reviewbybees done

Since we do lots of such renaming, we probably need to be aware of breaking method @since info in migrated methods (CC @daniel-beck). It is not a big deal though.

@jglick jglick added ready-for-merge and removed needs-review labels Nov 27, 2017

@oleg-nenashev oleg-nenashev merged commit 5f8f426 into jenkinsci:master Nov 28, 2017

1 check passed

continuous-integration/jenkins/pr-head This commit looks good
Details
@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Nov 28, 2017

Should this have deprecated now obsolete utility methods of Items?

@jglick

This comment has been minimized.

Member

jglick commented Nov 28, 2017

Should this have deprecated now obsolete utility methods of Items?

I considered it but decided to leave them as is, especially since the current default implementations continue to call them. No strong opinion.

@jglick jglick deleted the jglick:ItemGroup.allItems branch Nov 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment