Bugfix/forbid null items #706
Conversation
|
Due to the size of the change and integration testing required it won't be merged straight away. Just a heads up. |
Codecov Report
@@ Coverage Diff @@
## main #706 +/- ##
=======================================
Coverage 71.90% 71.90%
=======================================
Files 216 216
Lines 10893 10893
=======================================
Hits 7833 7833
Misses 3060 3060
|
I imagined as such. I don't think it should change any behaviour, but it could break existing code if it uses nullable types. |
|
I apologise for a long delay. I'd been pondering on whether we should do this, then I forgot ! The one thing which concerns me is that there is potentially a use case for null. A "fix" was introduced to enable it in the case of a transform to allow nulls. I believe the idea was to immediately filter the null out. Doing it this way enabled being able to suppress transforms in certain circumstances. @oysteinkrog can you remember why you did this, and why I approved - See #216 Regardless, nulls are still a bad idea and I wonder whether we should push this and perhaps introduce a mechanism to Dynamic Data where a null returned by a transform results in a remove event rather than a null update. |
|
|
||
| /// <summary> | ||
| /// Wraps the specified value in an Optional container. | ||
| /// </summary> | ||
| /// <typeparam name="T">The type of the item.</typeparam> | ||
| /// <param name="value">The value.</param> | ||
| /// <returns>The optional value.</returns> | ||
| public static Optional<T> Some<T>(T value) => new(value); | ||
| public static Optional<T> Some<T>(T? value) |
There was a problem hiding this comment.
Value could be null as the constructor of optional directly casts a null to a None.
There was a problem hiding this comment.
Doh. Ignore me as I just noticed the T?
Hmm yes, we used to do this as we have a Transform that is essentially using a Try-pattern, meaning that it may fail. |
|
Thanks for the input @oysteinkrog, that settles it. This PR is good to go. @glennawatson I'll approve this, but before we release I'd like to do a little house keeping within the codebase. I'll be done with that within a few days. |
|
Also @oysteinkrog I may additionally provide the option within the transform to return a null, in which case the item would be removed. |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
In response to #692, this PR adds a
where t : notnullconstraint toOption<T>.That change proliferates out to
IChangeSetand into a lot of extension methods.I think this constitutes a breaking change, but code that used nullable types may not have worked correctly anyway.