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

Add default implementations of deprecated methods of BuilableItem and Item. #3142

Merged
merged 3 commits into from Dec 1, 2017

Conversation

3 participants
@oleg-nenashev
Member

oleg-nenashev commented Nov 16, 2017

Currently the interface requires the API user to implement already deprecated methods.
It does not make much sense, and the API could be simplified.

Proposed changelog entries

  • N/A or "Developer: Introduce default implementations of deprecated methods of BuilableItem and Item"

Desired reviewers

@reviewbybees

Add default implementations to deprecated methods of BuilableItem and…
… Item.

Currently the interface requires the API user to implement already deprecated methods.
It does not make much sense, and the API could be simplified.
* @param useDisplayName if true, returns a display name, otherwise returns a name
* @return
* String like "foo » bar"
* String like "foo » bar".
* {@code null} if item is null or if one of its parents is not an {@link ItemGroup}.

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Nov 16, 2017

Member

Just documented it while I was around

@@ -40,13 +40,19 @@
* Use {@link #scheduleBuild(Cause)}. Since 1.283
*/
@Deprecated
boolean scheduleBuild();
default boolean scheduleBuild() {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Nov 16, 2017

Member

tabs vs spaces :( Should I use tabs or refactor the methods?

@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.

@oleg-nenashev oleg-nenashev requested a review from jglick Nov 16, 2017

@jglick

This comment has been minimized.

Member

jglick commented Nov 17, 2017

Fix your flaky tests. :-)

@jglick

jglick approved these changes Nov 17, 2017

@@ -1143,11 +1144,14 @@ private static String normalizeURI(String uri) {
*
* @since 1.515
* @param p the Item we want the relative display name
* @param g the ItemGroup used as point of reference for the item
* @param g the ItemGroup used as point of reference for the item.
* If the group is not specified, item's path will be used.

This comment has been minimized.

@jglick

jglick Nov 17, 2017

Member

mark it @CheckForNull please

* @param useDisplayName if true, returns a display name, otherwise returns a name
* @return
* String like "foo » bar"
* String like "foo » bar".
* {@code null} if item is null or if one of its parents is not an {@link ItemGroup}.

This comment has been minimized.

@jglick

jglick Nov 17, 2017

Member

Huh? A parent by definition is an ItemGroup. I guess you mean is not an Item?

@@ -1194,8 +1198,10 @@ public static String getRelativeNameFrom(Item p, ItemGroup g, boolean useDisplay
* @param p the Item we want the relative display name
* @param g the ItemGroup used as point of reference for the item
* @return
* String like "foo/bar"
* String like "foo/bar".
* {@code null} if the item is {@code null} or if one of its parents is not an {@link ItemGroup}.

This comment has been minimized.

@jglick

jglick Nov 17, 2017

Member

ditto

@@ -1208,8 +1214,10 @@ public static String getRelativeNameFrom(Item p, ItemGroup g) {
* @param p the Item we want the relative display name
* @param g the ItemGroup used as point of reference for the item
* @return
* String like "Foo » Bar"
* String like "Foo » Bar".
* {@code null} if the item is {@code null} or if one of its parents is not an {@link ItemGroup}.

This comment has been minimized.

@jglick

jglick Nov 17, 2017

Member

ditto

*/
@Nullable
public static String getRelativeNameFrom(Item p, ItemGroup g, boolean useDisplayName) {

This comment has been minimized.

@jglick

jglick Nov 17, 2017

Member

Really these methods should have been in Items (cf. #3148). Too late I suppose, unless we deprecate the Functions versions, which seems too intrusive.

* @since 1.419
* @return
* String like "../foo/bar"
* String like "../foo/bar".
* {@code null} if item parents is not an {@link ItemGroup}.

This comment has been minimized.

@jglick

jglick Nov 17, 2017

Member

probably s/ItemGroup/Item/

* @param g
* The ItemGroup instance used as context to evaluate the relative name of this AbstractItem
* @return
* The name of the current item, relative to p. Nested ItemGroups are separated by {@code /} character.

This comment has been minimized.

@jglick

jglick Nov 17, 2017

Member

use @link

@@ -131,18 +133,28 @@
/**
* Gets the relative name to this item from the specified group.
*
* @param g
* The ItemGroup instance used as context to evaluate the relative name of this AbstractItem

This comment has been minimized.

@jglick

jglick Nov 17, 2017

Member

Not necessarily an AbstractItem. Anyway use @link, or plain words.

@@ -207,7 +224,9 @@
*
* @since 1.374
*/
default void onCreatedFromScratch() {}
default void onCreatedFromScratch() {
// do nothing by default

This comment has been minimized.

@jglick

jglick Nov 17, 2017

Member

Why would there not be default no-op impls of onLoad and onCopiedFrom then?

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Nov 22, 2017

Member

I was thinking about that, but I decided not to touch it for now since I was unable to find noop implementations.

@oleg-nenashev oleg-nenashev requested a review from jglick Nov 27, 2017

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Nov 27, 2017

@jglick addressed your comments

@jglick

jglick approved these changes Nov 27, 2017

*/
default String getRelativeNameFrom(ItemGroup g) {
@Nullable

This comment has been minimized.

@jglick

jglick Nov 27, 2017

Member

Not actually sure why these are all @Nullable rather than @CheckForNull. Is there some circumstance where a caller would statically know the return value cannot be null?

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Nov 27, 2017

Member

Yes, generally "in all sane cases" assuming you do not resolve names for things like Promoted Builds plugin PromotionProcess instances or pass nulls. I can add @CheckForNull, but it may become just another Jenkins.getInstance()

@oleg-nenashev oleg-nenashev merged commit 8824857 into jenkinsci:master Dec 1, 2017

1 check passed

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

jglick added a commit to jglick/maven-plugin that referenced this pull request Jan 5, 2018

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