Fix List.MaximumItem and List.MinimumItem type promotion from Int to Double#17055
Fix List.MaximumItem and List.MinimumItem type promotion from Int to Double#17055
Conversation
…nstead of promoting to Double Agent-Logs-Url: https://github.com/DynamoDS/Dynamo/sessions/d56bc0ef-0f17-4d9e-afab-e75c43b8ee38 Co-authored-by: johnpierson <15744724+johnpierson@users.noreply.github.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| foreach (var item in list) | ||
| { | ||
| if (item == null) continue; | ||
| if (min == null || comparer.Compare(item, min) < 0) | ||
| min = item; | ||
| } |
| foreach (var item in list) | ||
| { | ||
| if (item == null) continue; | ||
| if (max == null || comparer.Compare(item, max) > 0) | ||
| max = item; | ||
| } |
|
There was a problem hiding this comment.
Pull request overview
Fixes List.MaximumItem / List.MinimumItem returning Double when the maximum/minimum element is an Int32/Int64, which can break strict-type downstream nodes. The change is implemented in DSCore.List and backed by new NUnit tests.
Changes:
- Replaced LINQ
Min/Maxwith manual iteration usingObjectComparerto return the original list item (preserving its runtime type). - Added unit tests asserting
Int32andInt64inputs keep their original types inMinimumItem/MaximumItem.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/Libraries/CoreNodes/List.cs |
Updates MinimumItem/MaximumItem selection logic to preserve original item type while still supporting cross-type numeric ordering. |
test/Libraries/CoreNodesTests/ListTests.cs |
Adds NUnit coverage to prevent regression of Int32/Int64 type preservation. |
| public static object MinimumItem(IEnumerable<object> list) | ||
| { | ||
| return list.Min<object, object>(DoubleConverter); | ||
| var comparer = new ObjectComparer(); | ||
| object min = null; | ||
| foreach (var item in list) | ||
| { |
There was a problem hiding this comment.
MinimumItem/MaximumItem now iterate with foreach (var item in list) but don't guard against list == null. If list is null this will throw NullReferenceException, whereas the previous LINQ implementation would throw an ArgumentNullException (and matches other LINQ-based nodes). Add an explicit if (list == null) throw new ArgumentNullException(nameof(list)); (or return null if that's the intended node contract) to avoid the regression.
| foreach (var item in list) | ||
| { | ||
| if (item == null) continue; | ||
| if (min == null || comparer.Compare(item, min) < 0) | ||
| min = item; |
There was a problem hiding this comment.
MinimumItem skips null entries (if (item == null) continue;). Previously, the LINQ-based implementation (Min(... DoubleConverter)) would treat a null projected value as the minimum and return null when any nulls are present in the sequence. This change can break graphs that relied on null propagating as the minimum. Either preserve the old semantics (return null when any element is null) or update the documented/expected behavior and add tests covering null-containing lists.
| foreach (var item in list) | |
| { | |
| if (item == null) continue; | |
| if (min == null || comparer.Compare(item, min) < 0) | |
| min = item; | |
| var hasMin = false; | |
| foreach (var item in list) | |
| { | |
| if (item == null) | |
| return null; | |
| if (!hasMin || comparer.Compare(item, min) < 0) | |
| { | |
| min = item; | |
| hasMin = true; | |
| } |
| var comparer = new ObjectComparer(); | ||
| object min = null; | ||
| foreach (var item in list) | ||
| { | ||
| if (item == null) continue; | ||
| if (min == null || comparer.Compare(item, min) < 0) | ||
| min = item; | ||
| } | ||
| return min; |
There was a problem hiding this comment.
The new loop returns null for an empty list (because min/max never gets assigned). The previous LINQ implementation (Enumerable.Min/Max) throws InvalidOperationException for empty sequences, so this is a behavioral change. If graphs/tests rely on the exception behavior, consider explicitly detecting empties and matching the prior behavior (or document/test the new null return contract).
| [Category("UnitTests")] | ||
| public static void ListMinimumItemPreservesIntType() | ||
| { | ||
| var result = List.MinimumItem(new List<object> { 8, 4, 0, 66, 10 }); |
There was a problem hiding this comment.
What happens when there is a mixed list of doubles and ints?
List.MaximumItemandList.MinimumItemwere silently promoting integer results (Int32/Int64) toDouble, breaking strict-type downstream nodes likeList.IndexOf.Root cause: Both methods used LINQ's
Min<TSource, TResult>(selector)/Max<TSource, TResult>(selector)with aDoubleConverterselector. These overloads return the projected value (aDouble), not the original list item.Fix:
src/Libraries/CoreNodes/List.cs— Replaced theDoubleConverter-based LINQ calls with foreach loops that useObjectComparerfor ordering. The original item is returned, preserving its type.ObjectCompareralready handles cross-type numeric comparisons (converting toDoubleinternally for ordering only).test/Libraries/CoreNodesTests/ListTests.cs— Added 4 unit tests asserting thatInt32andInt64inputs produce results of the same type.Empty and null-containing lists continue to return
null, consistent with the original LINQ behavior for reference-type results.Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
npm.autodesk.com/usr/bin/pwsh pwsh -ExecutionPolicy Bypass -File /home/REDACTED/work/Dynamo/Dynamo/src/setnpmreg.ps1(dns block)/usr/bin/pwsh pwsh -ExecutionPolicy ByPass -Command & '/home/REDACTED/work/Dynamo/Dynamo/src/setnpmreg.ps1'(dns block)If you need me to access, download, or install something from one of these locations, you can either: