-
Notifications
You must be signed in to change notification settings - Fork 37
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
Instrumentation registry changes #516
Instrumentation registry changes #516
Conversation
*/ | ||
interface AndroidInstrumentationRegistry { | ||
interface AndroidInstrumentationServices { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to say so, but I don't love this name. It's leaking out the fact that instrumentations are discovered via AutoService
(hence "Services") and suggests that the things you can get()
from it are services (but they're not, they're instrumentation). We also have the ServiceManager
, so putting Service
in the name implies a relationship there, where we might not want one.
Some alternative names:
AndroidInstrumentionDiscovery
DetectedInstrumentationLoader
InstrumentationFinder
AndroidInstrumentationLoader
AutoDiscoveredAndroidInstrumentationLoader
My preference is for the last one. It's more verbose, but I hope it's very clear in what it does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm always looking for name ideas, as that's the part I struggle the most with, so thanks for your suggestions!
I went with AndroidInstrumentationLoader
because I think it's the most concise. Since users might need to access this type to configure some instrumentations I think it's better to keep the name as short as possible.
* Convenience method for [AndroidInstrumentationServices.getByType]. | ||
*/ | ||
@JvmStatic | ||
fun <T : AndroidInstrumentation> getService(type: Class<out T>): T? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same concerns here about Service
. Would prefer to call this getInstrumentation()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, it's updated now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @LikeTheSalad , I appreciate where this is going. I am hung up on a couple of names, but otherwise seems good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Addresses #502