-
Notifications
You must be signed in to change notification settings - Fork 288
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
Allow library models to define custom stream classes. #807
Conversation
Pull Request Test Coverage Report for Build #1172
💛 - Coveralls |
/benchmark |
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.
Couple of nits and one genuine question, but overall this looks fine
// Note: Currently, OptimizedLibraryModels doesn't carry the information about stream type | ||
// records, | ||
// it is not clear what it means to "optimize" lookup for those and they get access only once: | ||
// here. |
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.
Nit: formatting
@@ -90,6 +92,10 @@ public StreamModelBuilder addStreamType(TypePredicate tp) { | |||
return this; | |||
} | |||
|
|||
public StreamModelBuilder addStreamTypeFromName(String fullyQualifiedName) { |
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.
Javadoc?
* <p>This handler must be registered in Handlers.java separatedly from the LibraryModelsHandler | ||
* itself. | ||
*/ | ||
public StreamNullabilityPropagator getStreamNullabilityPropagatorFromModels() { |
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.
It seems a bit awkward to put this method here as opposed to StreamNullabilityPropagatorFactory
. Is it here because it needs access to the libraryModels
object which is encapsulated within this class?
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.
Yeah, libraryModels
combines all loaded library models from auto-service, and I am not sure I want to provide a way to retrieve it from this class. What we could do is:
- Add
getCustomStreamNullabilitySpecs()
to this handler, which returnslibraryModels.customStreamNullabilitySpecs()
- Add a static method to
StreamNullabilityPropagatorFactory
which takes that and returns theStreamNullabilityPropagator
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 fine with whatever you think is cleanest
Main Branch:
With This PR:
|
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.
Going ahead and approving this as I don't think I need to check again about the previous comments
364adc6
to
1b4c6fe
Compare
This is needed to support both fully custom stream libraries, as well as backports/shadowed versions
of existing stream libraries.