WPF Binding improvements#565
Open
Daniel-Svensson wants to merge 3 commits intoOpenRIAServices:mainfrom
Open
Conversation
* Remove ListCollectionViewProxy class * Create ListCollectionView directly
Contributor
📝 WalkthroughWalkthroughThis pull request systematically replaces LINQ Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/OpenRiaServices.Client/Framework/EntityCollection.cs`:
- Around line 814-817: Add the same deprecation attribute to the
ICollectionViewFactory.CreateView implementation in EntityCollection (the
CreateView() method returning new ListCollectionView(...)) to match EntitySet:
mark CreateView() with [Obsolete(...)] using the same message used in
EntitySet's CreateView so callers see the planned removal; ensure the attribute
is applied to the ICollectionViewFactory.CreateView method declaration inside
EntityCollection<TEntity> and keep the implementation unchanged.
In `@src/OpenRiaServices.Client/Framework/EntitySet.cs`:
- Around line 1447-1451: Update the Obsolete attribute message on
ICollectionViewFactory.CreateView to guide users toward the recommended
replacement: mention that callers should use
CollectionViewSource.GetDefaultView(...) instead of relying on
ICollectionViewFactory, and state that implementing ICollectionViewFactory will
be removed in the next major version; locate the attribute on the
ICollectionViewFactory.CreateView method (which currently returns a new
ListCollectionView(this)) and replace the message text accordingly.
In
`@src/OpenRiaServices.Client/Test/Client.Test/Data/DomainOperationExceptionTests.cs`:
- Line 23: Replace the awkward "Assert.IsFalse(... Count != 0)" checks with
direct zero-count assertions for readability: in DomainOperationExceptionTests
replace assertions like Assert.IsFalse(doe.ValidationErrors.Count != 0, ...)
with Assert.AreEqual(0, doe.ValidationErrors.Count, ...) or
Assert.IsTrue(doe.ValidationErrors.Count == 0, ...) (preserve the original
message text if desired) for each occurrence referencing
doe.ValidationErrors.Count in the test methods.
In `@src/OpenRiaServices.Client/Test/Client.Test/Data/EntitySetTests.cs`:
- Around line 329-332: The tests currently use
CollectionViewSource.GetDefaultView(entitySet) in GetICV(EntitySet) to avoid the
obsolete ICollectionViewFactory.CreateView(); update the other test file where
code still casts to ICollectionViewFactory and calls CreateView() (the call in
EntityCollectionTests.cs that obtains an ICollectionView via CreateView) to
instead call CollectionViewSource.GetDefaultView(entitySet) (or centralize by
adding/using the same GetICV helper) so tests no longer rely on the obsolete
CreateView path.
In
`@src/OpenRiaServices.Client/Test/Client.Test/Data/SubmitOperationExceptionTests.cs`:
- Line 29: The assertions in SubmitOperationExceptionTests use
Assert.IsFalse(soe.ValidationErrors.Count != 0) which is harder to read; change
each to an explicit zero-count assertion (e.g., Assert.AreEqual(0,
soe.ValidationErrors.Count) or Assert.IsTrue(soe.ValidationErrors.Count == 0) )
for clarity. Update the three occurrences referencing soe.ValidationErrors.Count
in the SubmitOperationExceptionTests constructor/test methods (the assertions
around lines checking Count at 29, 40, and 51) so they directly assert zero
rather than negating a != 0 expression.
In `@src/Test/OpenRiaservices.EndToEnd.Wcf.Test/Data/ErrorPropagationTests.cs`:
- Line 256: The assertions use double negation
(Assert.IsFalse(zip.ValidationErrors.Count != 0)) which is less readable;
replace both occurrences with a direct empty check such as Assert.AreEqual(0,
zip.ValidationErrors.Count) (or Assert.IsTrue(zip.ValidationErrors.Count == 0))
to assert the ValidationErrors collection is empty, updating the two places that
reference zip.ValidationErrors.Count.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 10bb38c2-542a-4499-be74-f863ac0ef69a
📒 Files selected for processing (19)
src/OpenRiaServices.Client/Framework/ChangeSetBuilder.cssrc/OpenRiaServices.Client/Framework/ComplexObject.cssrc/OpenRiaServices.Client/Framework/Entity.cssrc/OpenRiaServices.Client/Framework/EntityCollection.cssrc/OpenRiaServices.Client/Framework/EntityQuery.cssrc/OpenRiaServices.Client/Framework/EntitySet.cssrc/OpenRiaServices.Client/Framework/Internal/MetaType.cssrc/OpenRiaServices.Client/Framework/InvokeOperation.cssrc/OpenRiaServices.Client/Framework/LoadOperation.cssrc/OpenRiaServices.Client/Framework/ObjectStateUtility.cssrc/OpenRiaServices.Client/Test/Client.Test/Data/DomainOperationExceptionTests.cssrc/OpenRiaServices.Client/Test/Client.Test/Data/EntitySetTests.cssrc/OpenRiaServices.Client/Test/Client.Test/Data/SubmitOperationExceptionTests.cssrc/OpenRiaServices.Server/Framework/Authentication/AuthenticationCodeProcessor.cssrc/OpenRiaServices.Server/Framework/Data/DomainServiceDescription.cssrc/Test/OpenRiaservices.EndToEnd.Wcf.Test/Data/DomainContextTests.cssrc/Test/OpenRiaservices.EndToEnd.Wcf.Test/Data/ErrorPropagationTests.cssrc/Test/OpenRiaservices.EndToEnd.Wcf.Test/Data/InvokeOperationTests.cssrc/Test/OpenRiaservices.EndToEnd.Wcf.Test/Data/ServerSideAsyncTests.cs
Comment on lines
814
to
817
| ICollectionView ICollectionViewFactory.CreateView() | ||
| { | ||
| // We use the CollectionViewSource to obtain a ListCollectionView, a type internal to Silverlight | ||
| return new CollectionViewSource() { Source = new ListCollectionViewProxy<TEntity>(this) }.View; | ||
| return new ListCollectionView(new ListCollectionViewProxy<TEntity>(this)); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider marking CreateView() as obsolete for consistency with EntitySet.
In EntitySet.cs, the ICollectionViewFactory.CreateView() method is marked [Obsolete] with a note about planned removal. For API consistency, consider applying the same deprecation here.
♻️ Suggested change for consistency
+ [Obsolete("ICollectionViewFactory is planned for removal in the next major version. Use CollectionViewSource.GetDefaultView() instead.")]
ICollectionView ICollectionViewFactory.CreateView()
{
return new ListCollectionView(new ListCollectionViewProxy<TEntity>(this));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ICollectionView ICollectionViewFactory.CreateView() | |
| { | |
| // We use the CollectionViewSource to obtain a ListCollectionView, a type internal to Silverlight | |
| return new CollectionViewSource() { Source = new ListCollectionViewProxy<TEntity>(this) }.View; | |
| return new ListCollectionView(new ListCollectionViewProxy<TEntity>(this)); | |
| } | |
| [Obsolete("ICollectionViewFactory is planned for removal in the next major version. Use CollectionViewSource.GetDefaultView() instead.")] | |
| ICollectionView ICollectionViewFactory.CreateView() | |
| { | |
| return new ListCollectionView(new ListCollectionViewProxy<TEntity>(this)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/OpenRiaServices.Client/Framework/EntityCollection.cs` around lines 814 -
817, Add the same deprecation attribute to the ICollectionViewFactory.CreateView
implementation in EntityCollection (the CreateView() method returning new
ListCollectionView(...)) to match EntitySet: mark CreateView() with
[Obsolete(...)] using the same message used in EntitySet's CreateView so callers
see the planned removal; ensure the attribute is applied to the
ICollectionViewFactory.CreateView method declaration inside
EntityCollection<TEntity> and keep the implementation unchanged.
Comment on lines
+1447
to
1451
| [Obsolete("Consider removing in next major version, not implementing ICollectionViewFactory should give the same behaviour")] | ||
| ICollectionView ICollectionViewFactory.CreateView() | ||
| { | ||
| // We use the CollectionViewSource to obtain a ListCollectionView, a type internal to Silverlight | ||
| return new CollectionViewSource() { Source = new ListCollectionViewProxy<TEntity>(this) }.View; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// <see cref="IList"/> proxy that makes the <see cref="EntitySet"/> usable in the default collection views. | ||
| /// All operations implemented against the proxy are passed through to the source <see cref="EntitySet"/>. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// This proxy does not support a full set of list operations. However, the subset it does support | ||
| /// is sufficient for interaction with the ListCollectionView. | ||
| /// </remarks> | ||
| /// <typeparam name="T">The entity type of this proxy</typeparam> | ||
| private class ListCollectionViewProxy<T> : IList, IEnumerable<T>, INotifyCollectionChanged, ICollectionChangedListener where T : Entity | ||
| { | ||
| private readonly object _syncRoot = new object(); | ||
| private readonly EntitySet<T> _source; | ||
| private readonly WeakCollectionChangedListener _weakCollectionChangedLister; | ||
|
|
||
| internal ListCollectionViewProxy(EntitySet<T> source) | ||
| { | ||
| this._source = source; | ||
| this._weakCollectionChangedLister = | ||
| WeakCollectionChangedListener.CreateIfNecessary(this._source, this); | ||
| } | ||
|
|
||
| #region IList | ||
|
|
||
| public int Add(object value) | ||
| => ((IList)Source).Add(value); | ||
|
|
||
| public void Clear() | ||
| { | ||
| this.Source.Clear(); | ||
| } | ||
|
|
||
| public bool Contains(object value) | ||
| { | ||
| return (value is Entity e) && this.Source.Contains(e); | ||
| } | ||
|
|
||
| public int IndexOf(object value) | ||
| { | ||
| return ((IList)this.Source.List).IndexOf(value); | ||
| } | ||
|
|
||
| public void Insert(int index, object value) | ||
| => ((IList)Source).Insert(index, value); | ||
|
|
||
| public bool IsFixedSize => ((IList)Source).IsFixedSize; | ||
|
|
||
| public bool IsReadOnly => ((IList)Source).IsReadOnly; | ||
|
|
||
| public void Remove(object value) | ||
| { | ||
| if (value is T entity) | ||
| { | ||
| this.Source.Remove(entity); | ||
| } | ||
| } | ||
|
|
||
| public void RemoveAt(int index) | ||
| { | ||
| Source.Remove(Source.List[index]); | ||
| } | ||
|
|
||
| public object this[int index] | ||
| { | ||
| get | ||
| { | ||
| return this.Source.List[index]; | ||
| } | ||
| set | ||
| { | ||
| throw new NotSupportedException( | ||
| string.Format(CultureInfo.CurrentCulture, Resource.IsNotSupported, "Indexed setting")); | ||
| } | ||
| } | ||
|
|
||
| public void CopyTo(Array array, int index) | ||
| { | ||
| ((IList)this.Source.List).CopyTo(array, index); | ||
| } | ||
|
|
||
| public int Count | ||
| { | ||
| get { return this.Source.Count; } | ||
| } | ||
|
|
||
| public bool IsSynchronized | ||
| { | ||
| get { return false; } | ||
| } | ||
|
|
||
| public object SyncRoot | ||
| { | ||
| get { return this._syncRoot; } | ||
| } | ||
|
|
||
| public IEnumerator<T> GetEnumerator() | ||
| { | ||
| return this.Source.GetEnumerator(); | ||
| } | ||
|
|
||
| IEnumerator IEnumerable.GetEnumerator() | ||
| { | ||
| return this.GetEnumerator(); | ||
| } | ||
|
|
||
| private EntitySet<T> Source | ||
| { | ||
| get { return this._source; } | ||
| } | ||
|
|
||
| #endregion | ||
|
|
||
| #region INotifyCollectionChanged | ||
|
|
||
| public event NotifyCollectionChangedEventHandler CollectionChanged; | ||
|
|
||
| private void OnCollectionChanged(NotifyCollectionChangedEventArgs e) | ||
| { | ||
| this.CollectionChanged?.Invoke(this, e); | ||
| } | ||
|
|
||
| private void OnSourceCollectionChanged(object sender, NotifyCollectionChangedEventArgs e) | ||
| { | ||
| this.OnCollectionChanged(e); | ||
| } | ||
|
|
||
| #endregion | ||
|
|
||
| #region ICollectionChangedListener | ||
|
|
||
| void ICollectionChangedListener.OnCollectionChanged(object sender, NotifyCollectionChangedEventArgs e) | ||
| { | ||
| this.OnSourceCollectionChanged(sender, e); | ||
| } | ||
|
|
||
| #endregion | ||
| return new ListCollectionView(this); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider improving the [Obsolete] message to guide users.
The current message suggests removing ICollectionViewFactory but doesn't direct users to the recommended alternative. Consider mentioning CollectionViewSource.GetDefaultView() as the replacement approach.
💡 Suggested improvement
- [Obsolete("Consider removing in next major version, not implementing ICollectionViewFactory should give the same behaviour")]
+ [Obsolete("ICollectionViewFactory is planned for removal in the next major version. Use CollectionViewSource.GetDefaultView() instead.")]
ICollectionView ICollectionViewFactory.CreateView()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| [Obsolete("Consider removing in next major version, not implementing ICollectionViewFactory should give the same behaviour")] | |
| ICollectionView ICollectionViewFactory.CreateView() | |
| { | |
| // We use the CollectionViewSource to obtain a ListCollectionView, a type internal to Silverlight | |
| return new CollectionViewSource() { Source = new ListCollectionViewProxy<TEntity>(this) }.View; | |
| } | |
| /// <summary> | |
| /// <see cref="IList"/> proxy that makes the <see cref="EntitySet"/> usable in the default collection views. | |
| /// All operations implemented against the proxy are passed through to the source <see cref="EntitySet"/>. | |
| /// </summary> | |
| /// <remarks> | |
| /// This proxy does not support a full set of list operations. However, the subset it does support | |
| /// is sufficient for interaction with the ListCollectionView. | |
| /// </remarks> | |
| /// <typeparam name="T">The entity type of this proxy</typeparam> | |
| private class ListCollectionViewProxy<T> : IList, IEnumerable<T>, INotifyCollectionChanged, ICollectionChangedListener where T : Entity | |
| { | |
| private readonly object _syncRoot = new object(); | |
| private readonly EntitySet<T> _source; | |
| private readonly WeakCollectionChangedListener _weakCollectionChangedLister; | |
| internal ListCollectionViewProxy(EntitySet<T> source) | |
| { | |
| this._source = source; | |
| this._weakCollectionChangedLister = | |
| WeakCollectionChangedListener.CreateIfNecessary(this._source, this); | |
| } | |
| #region IList | |
| public int Add(object value) | |
| => ((IList)Source).Add(value); | |
| public void Clear() | |
| { | |
| this.Source.Clear(); | |
| } | |
| public bool Contains(object value) | |
| { | |
| return (value is Entity e) && this.Source.Contains(e); | |
| } | |
| public int IndexOf(object value) | |
| { | |
| return ((IList)this.Source.List).IndexOf(value); | |
| } | |
| public void Insert(int index, object value) | |
| => ((IList)Source).Insert(index, value); | |
| public bool IsFixedSize => ((IList)Source).IsFixedSize; | |
| public bool IsReadOnly => ((IList)Source).IsReadOnly; | |
| public void Remove(object value) | |
| { | |
| if (value is T entity) | |
| { | |
| this.Source.Remove(entity); | |
| } | |
| } | |
| public void RemoveAt(int index) | |
| { | |
| Source.Remove(Source.List[index]); | |
| } | |
| public object this[int index] | |
| { | |
| get | |
| { | |
| return this.Source.List[index]; | |
| } | |
| set | |
| { | |
| throw new NotSupportedException( | |
| string.Format(CultureInfo.CurrentCulture, Resource.IsNotSupported, "Indexed setting")); | |
| } | |
| } | |
| public void CopyTo(Array array, int index) | |
| { | |
| ((IList)this.Source.List).CopyTo(array, index); | |
| } | |
| public int Count | |
| { | |
| get { return this.Source.Count; } | |
| } | |
| public bool IsSynchronized | |
| { | |
| get { return false; } | |
| } | |
| public object SyncRoot | |
| { | |
| get { return this._syncRoot; } | |
| } | |
| public IEnumerator<T> GetEnumerator() | |
| { | |
| return this.Source.GetEnumerator(); | |
| } | |
| IEnumerator IEnumerable.GetEnumerator() | |
| { | |
| return this.GetEnumerator(); | |
| } | |
| private EntitySet<T> Source | |
| { | |
| get { return this._source; } | |
| } | |
| #endregion | |
| #region INotifyCollectionChanged | |
| public event NotifyCollectionChangedEventHandler CollectionChanged; | |
| private void OnCollectionChanged(NotifyCollectionChangedEventArgs e) | |
| { | |
| this.CollectionChanged?.Invoke(this, e); | |
| } | |
| private void OnSourceCollectionChanged(object sender, NotifyCollectionChangedEventArgs e) | |
| { | |
| this.OnCollectionChanged(e); | |
| } | |
| #endregion | |
| #region ICollectionChangedListener | |
| void ICollectionChangedListener.OnCollectionChanged(object sender, NotifyCollectionChangedEventArgs e) | |
| { | |
| this.OnSourceCollectionChanged(sender, e); | |
| } | |
| #endregion | |
| return new ListCollectionView(this); | |
| } | |
| [Obsolete("ICollectionViewFactory is planned for removal in the next major version. Use CollectionViewSource.GetDefaultView() instead.")] | |
| ICollectionView ICollectionViewFactory.CreateView() | |
| { | |
| return new ListCollectionView(this); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/OpenRiaServices.Client/Framework/EntitySet.cs` around lines 1447 - 1451,
Update the Obsolete attribute message on ICollectionViewFactory.CreateView to
guide users toward the recommended replacement: mention that callers should use
CollectionViewSource.GetDefaultView(...) instead of relying on
ICollectionViewFactory, and state that implementing ICollectionViewFactory will
be removed in the next major version; locate the attribute on the
ICollectionViewFactory.CreateView method (which currently returns a new
ListCollectionView(this)) and replace the message text accordingly.
| Assert.AreEqual(0, doe.ErrorCode, "Error code s/b 0"); | ||
| Assert.AreEqual(OperationErrorStatus.ServerError, doe.Status, "Default status s/b ServerError"); | ||
| Assert.IsFalse(doe.ValidationErrors.Any(), "default validationErrors should be empty"); | ||
| Assert.IsFalse(doe.ValidationErrors.Count != 0, "default validationErrors should be empty"); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Simplify repeated empty checks for test readability.
On Line 23, Line 32, Line 41, Line 50, Line 59, Line 69, and Line 79, prefer direct Count == 0/AreEqual(0, Count) assertions over IsFalse(Count != 0).
Suggested cleanup pattern
- Assert.IsFalse(doe.ValidationErrors.Count != 0, "default validationErrors should be empty");
+ Assert.AreEqual(0, doe.ValidationErrors.Count, "default validationErrors should be empty");Also applies to: 32-32, 41-41, 50-50, 59-59, 69-69, 79-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/OpenRiaServices.Client/Test/Client.Test/Data/DomainOperationExceptionTests.cs`
at line 23, Replace the awkward "Assert.IsFalse(... Count != 0)" checks with
direct zero-count assertions for readability: in DomainOperationExceptionTests
replace assertions like Assert.IsFalse(doe.ValidationErrors.Count != 0, ...)
with Assert.AreEqual(0, doe.ValidationErrors.Count, ...) or
Assert.IsTrue(doe.ValidationErrors.Count == 0, ...) (preserve the original
message text if desired) for each occurrence referencing
doe.ValidationErrors.Count in the test methods.
Member
Author
There was a problem hiding this comment.
Let's resolve it later in a different pr
Contributor
There was a problem hiding this comment.
@Daniel-Svensson, understood! Would you like me to open a GitHub issue to track this cleanup (replacing Assert.IsFalse(... Count != 0) with Assert.AreEqual(0, ..., Count) in DomainOperationExceptionTests.cs) so it doesn't get lost?
🧠 Learnings used
Learnt from: Daniel-Svensson
Repo: OpenRIAServices/OpenRiaServices PR: 512
File: src/OpenRiaServices.Server/Test/DomainServiceDescriptionTest.cs:385-387
Timestamp: 2026-03-08T23:08:53.507Z
Learning: In OpenRiaServices code reviews, ensure that the AssociationTypeDescriptor compatibility wrapper in DomainTypeDescriptionProvider.GetTypeDescriptor is applied only when there is a custom TypeDescriptionProvider and it does NOT originate from an OpenRiaServices assembly with version 5.7.0 or later. For OpenRiaServices >= 5.7.0, entities already emit EntityAssociationAttribute and must not be double-wrapped. This guideline applies to C# source files under src (e.g., any DomainTypeDescriptionProvider usage checks, tests, or related providers).
Comment on lines
329
to
332
| private ICollectionView GetICV(EntitySet entitySet) | ||
| { | ||
| return ((ICollectionViewFactory)entitySet).CreateView(); | ||
| return CollectionViewSource.GetDefaultView(entitySet); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if EntityCollectionTests.cs also uses the ICollectionViewFactory pattern that should be updated
rg -n "ICollectionViewFactory" --type=cs -C3 src/OpenRiaServices.Client/Test/Repository: OpenRIAServices/OpenRiaServices
Length of output: 1826
🏁 Script executed:
#!/bin/bash
# Try to locate and check EntityCollectionTests.cs for ICollectionViewFactory usage
rg -n "ICollectionViewFactory" --type=cs src/ 2>/dev/null || echo "Search failed or file not found"Repository: OpenRIAServices/OpenRiaServices
Length of output: 2454
🏁 Script executed:
#!/bin/bash
# Get the specific lines in EntityCollectionTests.cs around the ICollectionViewFactory usage
sed -n '410,430p' src/OpenRiaServices.Client/Test/Client.Test/Data/EntityCollectionTests.csRepository: OpenRIAServices/OpenRiaServices
Length of output: 772
Good alignment with the deprecation of ICollectionViewFactory.CreateView().
Using CollectionViewSource.GetDefaultView() is the recommended WPF pattern and aligns with marking CreateView() as obsolete. Note that GetDefaultView() returns a cached view per source, which differs from CreateView() that creates fresh instances, but this shouldn't affect test isolation since each test creates a new EntitySet.
Consider also updating EntityCollectionTests.cs where GetICV() still casts to ICollectionViewFactory and calls CreateView() directly (line 419), matching the pattern used here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/OpenRiaServices.Client/Test/Client.Test/Data/EntitySetTests.cs` around
lines 329 - 332, The tests currently use
CollectionViewSource.GetDefaultView(entitySet) in GetICV(EntitySet) to avoid the
obsolete ICollectionViewFactory.CreateView(); update the other test file where
code still casts to ICollectionViewFactory and calls CreateView() (the call in
EntityCollectionTests.cs that obtains an ICollectionView via CreateView) to
instead call CollectionViewSource.GetDefaultView(entitySet) (or centralize by
adding/using the same GetICV helper) so tests no longer rely on the obsolete
CreateView path.
| Assert.AreEqual(0, soe.ErrorCode, "Error code s/b 0"); | ||
| Assert.AreEqual(OperationErrorStatus.Unauthorized, soe.Status, "ctor(msg, status) failed status"); | ||
| Assert.IsFalse(soe.ValidationErrors.Any(), "default validationErrors should be empty"); | ||
| Assert.IsFalse(soe.ValidationErrors.Count != 0, "default validationErrors should be empty"); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use clearer empty-check assertions in constructor tests.
On Line 29, Line 40, and Line 51, Assert.IsFalse(Count != 0) is harder to scan than direct zero-count assertions.
Suggested cleanup
- Assert.IsFalse(soe.ValidationErrors.Count != 0, "default validationErrors should be empty");
+ Assert.AreEqual(0, soe.ValidationErrors.Count, "default validationErrors should be empty");Also applies to: 40-40, 51-51
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/OpenRiaServices.Client/Test/Client.Test/Data/SubmitOperationExceptionTests.cs`
at line 29, The assertions in SubmitOperationExceptionTests use
Assert.IsFalse(soe.ValidationErrors.Count != 0) which is harder to read; change
each to an explicit zero-count assertion (e.g., Assert.AreEqual(0,
soe.ValidationErrors.Count) or Assert.IsTrue(soe.ValidationErrors.Count == 0) )
for clarity. Update the three occurrences referencing soe.ValidationErrors.Count
in the SubmitOperationExceptionTests constructor/test methods (the assertions
around lines checking Count at 29, 40, and 51) so they directly assert zero
rather than negating a != 0 expression.
| // Call RejectChanges and verify ValidationErrors collection is cleared | ||
| citiesProvider.RejectChanges(); | ||
| Assert.IsFalse(zip.ValidationErrors.Any()); | ||
| Assert.IsFalse(zip.ValidationErrors.Count != 0); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer direct empty assertions to avoid double negation.
On Line 256 and Line 270, Assert.IsFalse(...Count != 0) is correct but less readable than asserting Count == 0.
Suggested cleanup
- Assert.IsFalse(zip.ValidationErrors.Count != 0);
+ Assert.AreEqual(0, zip.ValidationErrors.Count);
...
- Assert.IsFalse(zip.ValidationErrors.Count != 0);
+ Assert.AreEqual(0, zip.ValidationErrors.Count);Also applies to: 270-270
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Test/OpenRiaservices.EndToEnd.Wcf.Test/Data/ErrorPropagationTests.cs` at
line 256, The assertions use double negation
(Assert.IsFalse(zip.ValidationErrors.Count != 0)) which is less readable;
replace both occurrences with a direct empty check such as Assert.AreEqual(0,
zip.ValidationErrors.Count) (or Assert.IsTrue(zip.ValidationErrors.Count == 0))
to assert the ValidationErrors collection is empty, updating the two places that
reference zip.ValidationErrors.Count.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| /// Represents a collection of <see cref="Entity"/> instances. | ||
| /// </summary> | ||
| public abstract class EntitySet : IEnumerable, ICollection, IList, INotifyCollectionChanged, IRevertibleChangeTracking, INotifyPropertyChanged | ||
| public abstract class EntitySet : IList, INotifyCollectionChanged, IRevertibleChangeTracking, INotifyPropertyChanged |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit