feature: Use IValidationState Instead of ValidationState #130
Conversation
Codecov Report
@@ Coverage Diff @@
## main #130 +/- ##
==========================================
+ Coverage 61.52% 61.57% +0.04%
==========================================
Files 16 16
Lines 863 877 +14
==========================================
+ Hits 531 540 +9
- Misses 332 337 +5
Continue to review full report at Codecov.
|
|
It's hard to keep up with you @worldbeater! I've had my head down on some other code today, so I only just noticed you'd pushed this. I've gone through and reviewed the code changes and I think you've struck the right balance between backwards compatibility and making, what I feel, is a fundamentally significant improvement. Not only will this positively impact extensibility it should impact performance. I look forward to the NuGet release so I can rip out a bunch of existing workaround code. I'll definitely use the |
|
Btw @thargy, how do you think, should we remove the generic version of
|
|
@worldbeater we're overlapping a bit. Just created a PR for Readme update (#131), but whilst doing it, I noticed that, now that public static readonly ValidationState Valid = new ValidationState(true, string.Empty);To I've made the necessary changes in #132. |
Proposed update to indicate use of `IValidationState` or `ValidationState` with `ValidationRule`.
Actually, having the interface would mean that the extension method will only work with As the extension method is largely only used internally, you get the problem where it might not be obvious to those implementing components why their component isn't working (i.e. they didn't explicitly implement the empty generic interface. Also, the generic type parameter would be unused in their component code. As such it isn't a promising idea IMHO, and I'd be in favour of dropping the generic interface entirely, as it is redundant.
This is the strongest argument, but I don't personally feel it justifies the potential confusion. Again, the extension method is largely used by the library, not consumers or implementors. If you wish to force a component to be bound to a view model type that should be part of the component design and logically it would require the generic interface to have a Although there are scenarios where having an empty interface is of use, in general, it is a strong indicator that a careful review is required to see if the interface actually provides value. Interfaces (and particularly generic ones) are not free to use. In this case, my view is that it is redundant and should be obsolesced.
|
Added static Valid property, for `ValidationState.Valid` as mentioned in reactiveui#130 (comment)
* Update ValidationState.cs Added static Valid property, for `ValidationState.Valid` as mentioned in #130 (comment) * Update README.md Updated document to use `ValidationState.Valid` as introduced in #131 * Replaced construction of valid validation states with `ValidationState.Valid`. * Added `ValidationState.Valid` to public API.
|
Going to spend some time on improving the public API surface of |
What kind of change does this PR introduce?
This PR implements the API specification eloquently argued and formulated by @thargy in #123 (comment)
What is the current behavior?
Currently, there is no way to customize a
ValidationState.Also, there is no
ValidationRuleoverload accepting aValidationState.See #123 for more information regarding this topic.
What is the new behavior?
Now, we are using
IValidationStatewhere possible, and this simplifies the use of complex and extended validation rules. Now we have two more overloads for theValidationRuleextension method, one of the rules accepts a view model andIObservable<IValidationState>, and another one accepts a view model, a property, and anIObservable<IValidationState>. This allows ReactiveUI.Validation users to fully control how an object implementingIValidtionStateis created.What might this PR break?
One potential breaking change is described in #123 (comment) e.g. there is no
.Componentproperty on the newIValidationStateinterface, so the code relying on that property might break. Hopefully, there aren't many people who are relying on that.Componentproperty. And if they do, a potential workaround would be to just castIObservable<IValidationState>toIObservable<ValidationState>by adding a call to.OfType<ValidationState>(), e.g:However, we discourage the use of
ValidationState.Componentobsolete property as we are going to remove it in the future.Please check if the PR fulfills these requirements
Other information:
We need to carefully review the API changes to avoid introducing additional breaking changes.