-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Null conditional operator in bindings #18270
base: master
Are you sure you want to change the base?
Conversation
We only had tests for the error states, and that was named incorrectly.
You can test this PR using the following package version. |
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.
There's a scenario that isn't handled properly currently: a type cast followed by a null conditional operator.
For example, {Binding ((SomeType)First)?.Second, TargetNullValue='null!'}
won't display the TargetNullValue
if First
is null
.
Edit: shouldn't be part of this PR, this is changing how the cast works, see comments below.
Apart from that and two very minor points, this looks good to me!
@@ -46,6 +46,20 @@ public bool TakeIf(char c) | |||
} | |||
} | |||
|
|||
public bool TakeIf(string s) |
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.
New public API, is that intended? (I don't think CharacterReader should have been exposed in the first place.)
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.
Gah, no that wasn't intended. I didn't expect that class to be public!
@@ -99,5 +113,17 @@ public void Skip(int count) | |||
throw new IndexOutOfRangeException(); | |||
_s = _s.Slice(count); | |||
} | |||
|
|||
private static bool SpanEquals(ReadOnlySpan<char> left, ReadOnlySpan<char> right) |
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 be replaced with the built-in ROS.SequenceEqual()
.
Some users might expect invalid cast exception here (and so, FallbackValue to be used instead). We don't have any ...but at the same time, "?" might be a good enough indicator in the binding syntax. |
I now realize that this isn't the current behavior of the binding cast at all: After this PR, the nuance becomes a bit more important. |
What does the pull request do?
As described in #17029 adds support for the null-conditional operator (
?.
) to reflection and compiled bindings.What is the current behavior?
If a null is encountered while evaluating in a binding path, then a warning will be logged and the binding will result in an error state. If such a binding is used in a multi-binding then the multi-binding will not produce a value as it will consider its input to be in an error state.
What is the updated/expected behavior with this PR?
One can now use the
?.
operator in a binding path to indicate "it is expected that this property may be null". If this is the case then the binding will producenull
instead of an error. An example:Indicates that it is expected that
Third
may be null.Note that this PR does not implement the related
?[]
operator for array indexing.Fixed issues
Fixes #17029