Skip to content
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

Make ValidationChangedObservable pipe pure #20

Merged
merged 9 commits into from
May 6, 2019
Merged

Make ValidationChangedObservable pipe pure #20

merged 9 commits into from
May 6, 2019

Conversation

worldbeater
Copy link
Collaborator

What kind of change does this PR introduce?

Fixes #19

What is the current behavior?

Currently, _validationSource and _validations won't stay synchronized due to the following code blocks:

.CountChanged()
.Count()
.StartWith(0)
.Select(_ =>
_validations
.Select(v => v.ValidationStatusChange)
Noticed DynamicData misuse here. The _validations collection is empty when validationChangedObservable ticks on a scheduler other than ImmediateScheduler. This happens because the validationConnectable pipe isn't pure — when using a scheduler other than immediate, the _validations read-only observable collection gets updated too late.

What is the new behavior?

Now, the validationConnectable is pure, and IsValid behaves correctly with the default scheduler.

What might this PR break?

All tests pass on my local machine, so probably nothing.

@worldbeater
Copy link
Collaborator Author

worldbeater commented May 6, 2019

Hmm, the CI checks failed, but it doesn't seem the PR caused the failure:

d:/a/1/s/tools/ReactiveUI.Cake.Recipe.1.0.103/parseanalyzeproject.cake(3,16): error CS0246: The type or namespace name 'CustomProjectParserResult' could not be found (are you missing a using directive or an assembly reference?)

BTW out of scope for this PR, but wondering if we support custom delimiters for validation messages:
image
If not, probably worth opening a new task and challenging it 🥇

@worldbeater worldbeater marked this pull request as ready for review May 6, 2019 23:12
@worldbeater worldbeater requested a review from a team May 6, 2019 23:12
@glennawatson
Copy link
Collaborator

I think you need to update the cake version. Use Punchcard or Splat as a example.

@glennawatson
Copy link
Collaborator

Hopefully those 3 changes get it.

@glennawatson
Copy link
Collaborator

Azure Devops has died without access to the internet. So will have to wait I guess.

@codecov
Copy link

codecov bot commented May 6, 2019

Codecov Report

Merging #20 into master will increase coverage by 0.18%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #20      +/-   ##
==========================================
+ Coverage   33.09%   33.28%   +0.18%     
==========================================
  Files          16       16              
  Lines         704      706       +2     
  Branches       82       82              
==========================================
+ Hits          233      235       +2     
  Misses        463      463              
  Partials        8        8
Impacted Files Coverage Δ
...eactiveUI.Validation/Contexts/ValidationContext.cs 80.95% <100%> (+0.62%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 21f3222...fddf068. Read the comment docs.

@glennawatson glennawatson merged commit 9f1395a into master May 6, 2019
@delete-merged-branch delete-merged-branch bot deleted the fix-19 branch May 6, 2019 23:58
@lock lock bot locked and limited conversation to collaborators Aug 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] ValidationContext is not producing updates.
2 participants