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 ExtensionList#lookupFirst convenience method #3021

Merged
merged 4 commits into from Oct 28, 2017

Conversation

4 participants
@daniel-beck
Member

daniel-beck commented Sep 12, 2017

I find myself typing ExtensionList.lookup(Whatever.class).get(Whatever.class) a lot to get the singleton instance for a registered @Extension.

If I'm not the only one (and haven't missed something obvious), maybe this is useful? @jenkinsci/code-reviewers More to tell me there's an obvious alternative I've missed.

@oleg-nenashev

Yes, it is something we really need. Probably makes sense to refactor bits within the core.

A more strict method would be also useful:

public static @CheckForNull <U,T> T lookup(Class<U> extensionType, Class<T> implementationType) {
        return lookup(extensionType).get(implementationType);
 }
*
* @param type The type to look up.
* @param <U>
* @return

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Sep 14, 2017

Member

Empty param/return should be removed or documented

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Sep 14, 2017

more strict method

But what would you even gain from that? This one simplifies things by getting rid of redundancy. Your proposal saves ~4 characters?

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Sep 14, 2017

But what would you even gain from that? This one simplifies things by getting rid of redundancy. Your proposal saves ~4 characters?

It also avoids the "YOLO" mode of lookupFirst (). But probably you could just remove "first" from the method name, because the call guarantees to have only one result IIUC

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Sep 14, 2017

because the call guarantees to have only one result IIUC

No, as I may pass e.g. Builder.class.

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Sep 15, 2017

Builder... Uh oh, it should not be stored in the extension implementation list imho. But if it is true now, then the current method naming is correct of course

@oleg-nenashev

Javadoc still needs to be fixed

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Sep 15, 2017

@oleg-nenashev

it should not be stored in the extension implementation list imho

Good point, then whatever the Descriptor type for that is, if any. The point is, the list can be any @Extension type, including super types, from a single implementation to e.g. Descriptor. To get a singleton instance, you may lookup(Super).get(Specific) or just lookup(Specific).get(Specific).

Notably, lookup(Super).get(Super) already works and just gets whatever entry first fits.

@daniel-beck daniel-beck removed the needs-fix label Sep 28, 2017

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Sep 28, 2017

@daniel-beck daniel-beck requested a review from jglick Sep 28, 2017

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Sep 28, 2017

This is the kind of change where I expect @jglick to tell me it's stupid and redundant due to something I didn't know about, so asking him for review.

@amuniz

amuniz approved these changes Sep 29, 2017

👍

@jglick

Needs some sample calls, for example of Uptime from AbstractProject._poll.

*
* @since TODO
*/
public static @CheckForNull <U> U lookupFirst(Class<U> type) {

This comment has been minimized.

@jglick

jglick Sep 29, 2017

Member

@CheckForNull here more or less defeats the real need for this, which is for cases where you expect that there must be one instance. I would rather say something like

ExtensionList<U> all = lookup(type);
if (all.size() != 1) throw new IllegalStateException(…);
return all.get(0);

And name it something like lookUpSingleton.

See for example GlobalConfigurationCategory.get, Jenkins.doConfigExecutorsSubmit call to MasterBuildConfiguration, JenkinsLocationConfiguration.get, UserDetailsCache.get, ad nauseam. Basically just Find Usages on ExtensionList.get(Class). If you know that the singleton has been registered, then you expect to be able to load it without null checks. Maybe existing calls in fact just assume that the result is not null, meaning FindBugs would complain if we actually let it.

public static @Nonnull <U> U lookupSingleton(Class<U> type) {
ExtensionList<U> all = lookup(type);
if (all.size() != 1) {
throw new IllegalStateException("Expected 1 instance of " + type.getName() + " but got " + all.size());

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Oct 3, 2017

Member

Needs to be Javadoced. throws would be also useful.
In the case of this method, I agree that RuntimeException is a right approach.

This comment has been minimized.

@amuniz

amuniz Oct 3, 2017

Member

Javadoced 😄 Love English language.

This comment has been minimized.

@daniel-beck

daniel-beck Oct 3, 2017

Member

@oleg-nenashev It's mentioned in https://github.com/jenkinsci/jenkins/pull/3021/files#diff-e3f986a0854b9b724f4e12690cdeeac3R423 and I see no point in adding it to the signature. It's a runtime exception after all.

This comment has been minimized.

@jglick

jglick Oct 18, 2017

Member

Still a good practice; helpful for Javadoc viewers.

/**
 * …
 * @throws IllegalStateException if there are no instances, or more than one
 */
@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Oct 3, 2017

I see no point in adding it to the signature. It's a runtime exception after all.

@daniel-beck The only point of it is to have just another place where the API users are warned about that in IDE. Nothing else

@oleg-nenashev

OK though @throws tag in Javadoc exits for a reason. 🐝

@daniel-beck daniel-beck requested a review from jglick Oct 13, 2017

@jglick

jglick approved these changes Oct 18, 2017

public static @Nonnull <U> U lookupSingleton(Class<U> type) {
ExtensionList<U> all = lookup(type);
if (all.size() != 1) {
throw new IllegalStateException("Expected 1 instance of " + type.getName() + " but got " + all.size());

This comment has been minimized.

@jglick

jglick Oct 18, 2017

Member

Still a good practice; helpful for Javadoc viewers.

/**
 * …
 * @throws IllegalStateException if there are no instances, or more than one
 */
if (mbc!=null)
mbc.configure(req,json);
MasterBuildConfiguration mbc = ExtensionList.lookupSingleton(MasterBuildConfiguration.class);
mbc.configure(req,json);

This comment has been minimized.

@jglick

jglick Oct 18, 2017

Member

or even inline var

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Oct 19, 2017

Addressed @jglick's review comments (including one identical to one by @oleg-nenashev ), so I think this is ready to go into the next weekly.

@oleg-nenashev

🐝 Ready to go.

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Oct 27, 2017

retriggering CI

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Oct 27, 2017

@daniel-beck Sorry, I've forgotten to reopen it :( Was in my TODO list for the next weekly anyway

@jglick

jglick approved these changes Oct 27, 2017

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Oct 27, 2017

CI hates us :(

@jglick jglick referenced this pull request Oct 27, 2017

Merged

[JENKINS-31661] Check the root url (Admin monitor + Wizard) #3082

3 of 3 tasks complete
@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Oct 28, 2017

🚢 🇮🇹

@oleg-nenashev oleg-nenashev merged commit 671a0ad into jenkinsci:master Oct 28, 2017

1 check failed

continuous-integration/jenkins/pr-head This commit has test failures
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment