[JEP-216] Add SPI for localized resource lookup #156
Conversation
| * | ||
| * <ul> | ||
| * <li>A full URL represented as string. In Jenkins this is typically plugin webapp resources.</li> | ||
| * <li>A path with leading slash. These are actual webapp resources, with that being the implicit base directory.</li> |
There was a problem hiding this comment.
A potentially better design would be to have independent methods lookupUrl and lookupPath. Thoughts?
There was a problem hiding this comment.
Can't figure out the use cases. The interface method may not need to take care of those details.
There was a problem hiding this comment.
I'm generally in favor of using explicitly named separate methods when there is a clear separation of responsibility or implementation. In this case, though, I'm not sure that separation exists. Both forms are valid types of URLs.
Is there some reason we think the caller or implementor might need to handle these two forms differently or separately?
There was a problem hiding this comment.
Is there some reason we think the caller or implementor might need to handle these two forms differently or separately?
It's most of what https://github.com/daniel-beck/localization-support-plugin/blob/master/src/main/java/io/jenkins/plugins/localization/support/stapler/LocaleDrivenResourceProviderImpl.java does.
"From a plugin or not" is the same distinction as the two variants documented here.
There was a problem hiding this comment.
Both forms are valid types of URLs.
Note that the second one (plugin resources) is just /path/on/file/system pointing to $JENKINS_HOME/plugins/<ID>/….
|
|
||
| private static final Logger LOGGER = Logger.getLogger(LocaleDrivenResourceProvider.class.getName()); | ||
|
|
||
| private static synchronized List<LocaleDrivenResourceProvider> getLocaleDrivenResourceProviders() { |
There was a problem hiding this comment.
You can limit the synchronization to after you check for null (but then check for null again). Make the field volatile and you've got a threadsafe lazy initialized value.
There was a problem hiding this comment.
I'm going with @jglick's verbatim advice from an earlier equivalent change here, and not touching this 😃
| * | ||
| * <ul> | ||
| * <li>A full URL represented as string. In Jenkins this is typically plugin webapp resources.</li> | ||
| * <li>A path with leading slash. These are actual webapp resources, with that being the implicit base directory.</li> |
There was a problem hiding this comment.
I'm generally in favor of using explicitly named separate methods when there is a clear separation of responsibility or implementation. In this case, though, I'm not sure that separation exists. Both forms are valid types of URLs.
Is there some reason we think the caller or implementor might need to handle these two forms differently or separately?
|
|
||
| private static List<LocaleDrivenResourceProvider> localeDrivenResourceProviders; | ||
|
|
||
| private static final Logger LOGGER = Logger.getLogger(LocaleDrivenResourceProvider.class.getName()); |
There was a problem hiding this comment.
nit: field declarations should be done before methods declarations
| * | ||
| * This cannot be made a property of WebApp as other behavior customizations, as webapp resource lookup is done before we have StaplerRequest/StaplerResponse. | ||
| */ | ||
| public abstract class LocaleDrivenResourceProvider { |
There was a problem hiding this comment.
Add the implements ExtensionPoint ?
There was a problem hiding this comment.
Can't, that's in Jenkins.
There was a problem hiding this comment.
| localeDrivenResourceProviders = new ArrayList<>(); | ||
| for (LocaleDrivenResourceProvider provider : ServiceLoader.load(LocaleDrivenResourceProvider.class)) { | ||
| localeDrivenResourceProviders.add(provider); | ||
| LOGGER.log(Level.INFO, "Registered LocaleDrivenResourceProvider: " + provider); |
There was a problem hiding this comment.
Probably could be made FINE since this is not exactly interesting news to an admin to be shown on every startup.
Upstream of jenkinsci/jenkins#3729 etc.
Specifically, allows daniel-beck/localization-support-plugin@a5b4e4f to fix daniel-beck/localization-support-plugin#1.
JEP-216
@LinuxSuRen