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

Use Space char as default delimiter #21

Merged
merged 1 commit into from
May 7, 2019
Merged

Conversation

worldbeater
Copy link
Collaborator

@worldbeater worldbeater commented May 7, 2019

What kind of change does this PR introduce?

Now SingleLineFormatter uses the " " character by default for validation error messages concatenation. I think this behavior is what a new user would expect from the library.

What is the current behavior?

Before, it just concatenated the error message texts. This caused the messages to be glued to each other.

image

What is the new behavior?

Now the messages are concatenated using the whitespace character. This can be easily overriden by passing a custom SingleLineFormatter to BindValidation, but probably using the whitespace character out-of-box will make the library more developer-friendly.

image

What might this PR break?

Sometimes when running unit tests locally I get the following exception thrown in random tests:

System.InvalidOperationException : Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct.
   at System.Collections.Generic.Dictionary`2.TryInsert(TKey key, TValue value, InsertionBehavior behavior)
   at ReactiveUI.POCOObservableForProperty.GetNotificationForProperty(Object sender, Expression expression, String propertyName, Boolean beforeChanged) in D:\a\1\s\src\ReactiveUI\ObservableForProperty\POCOObservableForProperty.cs:line 39
   at ReactiveUI.ReactiveNotifyPropertyChangedMixin.NotifyForProperty(Object sender, Expression expression, Boolean beforeChange) in D:\a\1\s\src\ReactiveUI\Mixins\ReactiveNotifyPropertyChangedMixin.cs:line 206
   at ReactiveUI.ReactiveNotifyPropertyChangedMixin.NestedObservedChanges(Expression expression, IObservedChange`2 sourceChange, Boolean beforeChange) in D:\a\1\s\src\ReactiveUI\Mixins\ReactiveNotifyPropertyChangedMixin.cs:line 190
   at System.Reactive.Linq.ObservableImpl.Select`2.Selector._.OnNext(TSource value) in D:\a\1\s\Rx.NET\Source\src\System.Reactive\Linq\Observable\Select.cs:line 39

This seem to happen due to a race condition in unit tests harness. The dictionary in POCOObservableForProperty is not concurrent, probably we should fix that:
https://github.com/reactiveui/ReactiveUI/blob/eb53eee4bd1865b3a226ec468c0a344620349bee/src/ReactiveUI/ObservableForProperty/POCOObservableForProperty.cs#L21

@worldbeater worldbeater added the enhancement New feature or request label May 7, 2019
@worldbeater worldbeater marked this pull request as ready for review May 7, 2019 21:56
@worldbeater worldbeater requested a review from a team May 7, 2019 21:56
@codecov
Copy link

codecov bot commented May 7, 2019

Codecov Report

Merging #21 into master will increase coverage by 1.69%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #21      +/-   ##
==========================================
+ Coverage   33.28%   34.98%   +1.69%     
==========================================
  Files          16       16              
  Lines         706      706              
  Branches       82       82              
==========================================
+ Hits          235      247      +12     
+ Misses        463      451      -12     
  Partials        8        8
Impacted Files Coverage Δ
...iveUI.Validation/Formatters/SingleLineFormatter.cs 85.71% <100%> (ø) ⬆️
...eactiveUI.Validation/Contexts/ValidationContext.cs 84.12% <0%> (+3.17%) ⬆️
...ctiveUI.Validation/Extensions/ViewForExtensions.cs 17.64% <0%> (+5.88%) ⬆️
...Validation/ValidationBindings/ValidationBinding.cs 33.67% <0%> (+9.18%) ⬆️

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 076d5ef...f5373a9. Read the comment docs.

@glennawatson
Copy link
Collaborator

Does doing new line as the delimiter work?

@glennawatson
Copy link
Collaborator

Not suggesting changing that as default but I would want that as a option

@worldbeater
Copy link
Collaborator Author

Yeah, Environment.NewLine worked fine for WPF.

image

Not sure about other platforms btw.

@glennawatson glennawatson merged commit b6e0a32 into master May 7, 2019
@delete-merged-branch delete-merged-branch bot deleted the add-whitespace branch May 7, 2019 22:09
@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
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants