-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[BEAM-551] Add NestedValueProvider #1146
Conversation
|
||
/** | ||
* An interface that provides a translator for a {@link NestedValueProvider} | ||
* to produce a value from a deferred {@link ValueProvider}. |
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.
we should pick some consistent logic about these names. Are they "deferrable" or "runtime" or something else?
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.
ack, should use runtime
|
||
@Nullable | ||
private final ValueProvider<X> value; | ||
private final DeferrableTranslator<T, X> translator; |
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.
Can we just use SerializableFunction
for this?
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.
Um, yes, indeed.
|
||
NestedValueProvider(@Nullable ValueProvider<X> value, DeferrableTranslator<T, X> translator) { | ||
this.value = value; | ||
this.translator = translator; |
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.
checkNotNull
the non-nullable value.
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.
(Maybe I'm missing something, but why is the VP allowed to be null
?)
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.
Done
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.
Please take another look.
|
||
/** | ||
* An interface that provides a translator for a {@link NestedValueProvider} | ||
* to produce a value from a deferred {@link ValueProvider}. |
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.
ack, should use runtime
|
||
@Nullable | ||
private final ValueProvider<X> value; | ||
private final DeferrableTranslator<T, X> translator; |
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.
Um, yes, indeed.
|
||
NestedValueProvider(@Nullable ValueProvider<X> value, DeferrableTranslator<T, X> translator) { | ||
this.value = value; | ||
this.translator = translator; |
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.
Done
* {@link NestedValueProvider} is an implementation of {@link ValueProvider} that | ||
* allows for wrapping another {@link ValueProvider} object. | ||
*/ | ||
public static class NestedValueProvider<T, X> implements ValueProvider<T>, Serializable { |
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.
ValueProvider.java:100:3: Redundant 'public' modifier. [RedundantModifier]
checkstyle :/
Manually fixed the checkstyle issue and merged. |
Wrote this up based on some nascent thoughts, and looking ahead to PubsubIO, which uses a bunch of wrapper classes instead of String. This allows us to nicely turn those into VP instead of deferring all the translation code into the caller, which would be gross.
R: @dhalperi