Skip to content
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

1.x: add shorter RxJavaPlugin class lookup approach. #3513

Merged
merged 1 commit into from
Nov 12, 2015

Conversation

akarnokd
Copy link
Member

@akarnokd akarnokd commented Nov 9, 2015

This adds a new pattern and lookup method that let's the developer specify the custom plugins with shorter system property keys. Android is quite restrictive and allows only 31 characters.

The new pattern splits the target simple class name and its implementation into two separate system properties:

rxjava.plugin.1.class=SimpleClassName
rxjava.plugin.1.impl=path.to.impl.Class

The index tag (1) can be any string of your chosing:

rxjava.plugin.mykey.class=SimpleClassName
rxjava.plugin.mykey.impl=path.to.impl.Class

but make sure they are paired, otherwise nothing will happen.

If there are multiple class entries with the same SimpleClassName one of them will be chosen (depending on the walk order in java.util.Properties).

Related issue #2835.

@akarnokd akarnokd added this to the 1.0.x milestone Nov 9, 2015
@akarnokd akarnokd changed the title 1.x: add shorter RxJavaPlugin class lockup approach. 1.x: add shorter RxJavaPlugin class lookup approach. Nov 10, 2015
@stealthcode
Copy link

👍

@@ -141,7 +156,7 @@ public void registerObservableExecutionHook(RxJavaObservableExecutionHook impl)
}
}

private static Object getPluginImplementationViaProperty(Class<?> pluginClass) {
/* test */ static Object getPluginImplementationViaProperty(Class<?> pluginClass, Properties props) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change test to VisibleForTesting? It's pretty common pattern.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find this VisibleForTesting in RxJava.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VisibleForTesting is in Guava

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant not annotation but comment.

On Wed, Nov 11, 2015, 21:32 Shixiong Zhu [email protected] wrote:

In src/main/java/rx/plugins/RxJavaPlugins.java
#3513 (comment):

@@ -141,7 +156,7 @@ public void registerObservableExecutionHook(RxJavaObservableExecutionHook impl)
}
}

  • private static Object getPluginImplementationViaProperty(Class<?> pluginClass) {
  • /* test */ static Object getPluginImplementationViaProperty(Class<?> pluginClass, Properties props) {

VisibleForTesting is in Guava


Reply to this email directly or view it on GitHub
https://github.com/ReactiveX/RxJava/pull/3513/files#r44567271.

@artem_zin

@akarnokd
Copy link
Member Author

Updated: made strings local constants and using length˙instead of magic numbers. Plus, if the default keying matches, the properties is not traversed for an alternate specification.

@akarnokd
Copy link
Member Author

An alternative pattern could be this:

rxjava.plugin.1=SimpleClassName,org.package.naming.ActualClass

@stealthcode
Copy link

I agree that if the plugin cannot be found we should crash the app.

@akarnokd
Copy link
Member Author

Updated the code to crash if the plugin implementation is missing.

@stealthcode
Copy link

Thanks!

akarnokd added a commit that referenced this pull request Nov 12, 2015
1.x: add shorter RxJavaPlugin class lookup approach.
@akarnokd akarnokd merged commit 1af0722 into ReactiveX:1.x Nov 12, 2015
@akarnokd akarnokd deleted the ShorterPluginNames branch November 12, 2015 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants