Skip to content

Fix for distinct calculator with duplicate keys - #274

Merged
RolandPheasant merged 2 commits into
masterfrom
unknown repository
Sep 20, 2019
Merged

Fix for distinct calculator with duplicate keys#274
RolandPheasant merged 2 commits into
masterfrom
unknown repository

Conversation

@ghost

@ghost ghost commented Sep 20, 2019

Copy link
Copy Markdown

What kind of change does this PR introduce?
Fixes #273

What is the current behavior?
Exception with duplicate keys as described in issue

What is the new behavior?
Exception resolved, new unit test added

What might this PR break?
No change to existing functionality, all unit tests pass

Please check if the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

Other information:
Not sure what documentation I need to update for this bug fix

@reactivemarbles reactivemarbles deleted a comment from dnfclas Sep 20, 2019
@glennawatson

Copy link
Copy Markdown
Member

Just some analyzers picking up some errors there

Cache\Internal\DistinctCalculator.cs(54,39): error RCS1037: Remove trailing white-space. [d:\a\1\s\src\DynamicData\DynamicData.csproj]
Cache\Internal\DistinctCalculator.cs(62,35): error RCS1037: Remove trailing white-space. [d:\a\1\s\src\DynamicData\DynamicData.csproj]
List\Internal\DynamicCombiner.cs(139,31): warning CA2208: Method UpdateResultList passes 'Reason' as the paramName argument to a ArgumentOutOfRangeException constructor. Replace this argument with one of the method's parameter names. Note that the provided parameter name should have the exact casing as declared on the method. [d:\a\1\s\src\DynamicData\DynamicData.csproj]
List\Internal\Combiner.cs(127,31): warning CA2208: Method UpdateResultList passes 'Reason' as the paramName argument to a ArgumentOutOfRangeException constructor. Replace this argument with one of the method's parameter names. Note that the provided parameter name should have the exact casing as declared on the method. [d:\a\1\s\src\DynamicData\DynamicData.csproj]
Cache\Internal\DistinctCalculator.cs(54,39): error RCS1037: Remove trailing white-space. [d:\a\1\s\src\DynamicData\DynamicData.csproj]
Cache\Internal\DistinctCalculator.cs(62,35): error RCS1037: Remove trailing white-space. [d:\a\1\s\src\DynamicData\DynamicData.csproj]
List\Internal\DynamicCombiner.cs(139,31): warning CA2208: Method UpdateResultList passes 'Reason' as the paramName argument to a ArgumentOutOfRangeException constructor. Replace this argument with one of the method's parameter names. Note that the provided parameter name should have the exact casing as declared on the method. [d:\a\1\s\src\DynamicData\DynamicData.csproj]
List\Internal\Combiner.cs(127,31): warning CA2208: Method UpdateResultList passes 'Reason' as the paramName argument to a ArgumentOutOfRangeException constructor. Replace this argument with one of the method's parameter names. Note that the provided parameter name should have the exact casing as declared on the method. [d:\a\1\s\src\DynamicData\DynamicData.csproj]
Cache\Internal\DistinctCalculator.cs(54,39): error RCS1037: Remove trailing white-space. [d:\a\1\s\src\DynamicData\DynamicData.csproj]
Cache\Internal\DistinctCalculator.cs(62,35): error RCS1037: Remove trailing white-space. [d:\a\1\s\src\DynamicData\DynamicData.csproj]
List\Internal\Combiner.cs(127,31): warning CA2208: Method UpdateResultList passes 'Reason' as the paramName argument to a ArgumentOutOfRangeException constructor. Replace this argument with one of the method's parameter names. Note that the provided parameter name should have the exact casing as declared on the method. [d:\a\1\s\src\DynamicData\DynamicData.csproj]
List\Internal\DynamicCombiner.cs(139,31): warning CA2208: Method UpdateResultList pas

@ghost

ghost commented Sep 20, 2019

Copy link
Copy Markdown
Author

Apologies, have removed trailing whitespace

@glennawatson

Copy link
Copy Markdown
Member

Roland has been away lately so if he doesn't respond in the next day, I will get some other Dynamic Data regulars and myself to do a review.

@glennawatson glennawatson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me. Thanks for having the unit test and the issue to understand what's going on.

@ghost

ghost commented Sep 20, 2019

Copy link
Copy Markdown
Author

Thanks Glenn, I think this makes the DistinctCalculator more robust, perhaps for scenarios that weren't envisaged originally like duplicate keys.

My use case is I'm trying to produce a list of distinct child items from a list of parents where there is some overlap in the child items between parents.

ParentCache
.Connect()
.MergeMany(p => p.ChildrenCache.Connect())
.DistinctValues(c => c)

This is where I came across the issue in the first place. Investigating further this is very similar to the Or logical combiners but I need this to be dynamic as the parentCache changes

@RolandPheasant

Copy link
Copy Markdown
Collaborator

This PR is great thanks and no reason not to merge.

Incidentally, the example code as stated is similar to using Or and that you have used MergeMany to compensate for a missing dynamic Or operator (or TransformMany) operator. There is a need for me to add new overloads of Or and TransformMany. Regardless, this fix will also apply when those operators have been implemented.

@RolandPheasant
RolandPheasant merged commit d6c660b into reactivemarbles:master Sep 20, 2019
@ghost
ghost deleted the distinct-calculator-fix branch September 24, 2019 03:09
@lock lock Bot locked and limited conversation to collaborators Dec 23, 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] DistinctCalculator bug with duplicate items (keys)

2 participants