fix #1500: строгое сравнение значений при поиске в коллекциях #1502
Conversation
WalkthroughThis pull request introduces a new static class Changes
Sequence Diagram(s)sequenceDiagram
participant F as ArrayImpl.Find
participant S as IValueExtension.StrictEquals
participant V as IValue instance
F->>+S: Call StrictEquals(value, target)
S-->>-F: Return boolean result
Possibly related PRs
Poem
Tip 🌐 Web search-backed reviews and chat
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/OneScript.Core/Values/IValue.cs (1)
21-25: LGTM! Consider adding XML documentation.The extension method is well-implemented, correctly using
IEquatable<IValue>.Equalsand comparingSystemType. The implementation is concise and immutable.Add XML documentation to describe the method's purpose and parameters:
+ /// <summary> + /// Performs a strict equality comparison between two IValue instances by comparing both their values and SystemType. + /// </summary> + /// <param name="value">The source IValue instance.</param> + /// <param name="other">The IValue instance to compare with.</param> + /// <returns>True if both values and their SystemTypes are equal; otherwise, false.</returns> public static bool StrictEquals(this IValue value, IValue other) => value.Equals(other) && value.SystemType == other.SystemType;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/OneScript.Core/Values/IValue.cs(1 hunks)src/OneScript.StandardLibrary/Collections/ArrayImpl.cs(1 hunks)
🔇 Additional comments (1)
src/OneScript.StandardLibrary/Collections/ArrayImpl.cs (1)
113-125: LGTM! Consider adding test cases.The change correctly implements strict value comparison in the
Findmethod using the newStrictEqualsextension method.Please add test cases to verify the behavior with values of different
SystemTypebut equal value. For example:[Test] public void Find_WithDifferentSystemType_ReturnsEmpty() { var array = new ArrayImpl(); array.Add(ValueFactory.Create(42)); // Number var result = array.Find(ValueFactory.Create("42")); // String Assert.That(result.AsNumber(), Is.EqualTo(0)); // Empty value }✅ Verification successful
LGTM! Consider adding test cases.
TheFindmethod correctly usesStrictEqualsin the lambda passed toFindIndexto compare values strictly, and it appropriately returns an empty value when no match is found while returning the index when a match is found. To further ensure robustness, please add test cases—especially ones that check behavior for values with the same content but differing inSystemType(e.g., a number versus a string). For instance:
- Verify that searching for a string
"42"in an array containing a number42returns an empty value.- Additional tests with other type combinations might further solidify the method’s reliable behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts or web queries found
|
Потенциально подвержены еще ТаблицаЗначений/ДеревоЗначений и СписокЗначений |
…х значений +тесты
Mr-Rm
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/OneScript.Core/Values/BslBooleanValue.cs (1)
61-69: Improved equality comparison implementation.The changes enhance readability and maintainability by:
- Using modern C# pattern matching for null checks
- Using switch expression for cleaner type-specific comparisons
- Maintaining backward compatibility with numeric comparisons
Consider adding XML documentation to explain the equality behavior, especially the numeric comparison case:
+ /// <summary> + /// Determines whether the specified value is equal to the current value. + /// </summary> + /// <param name="other">The value to compare with the current value.</param> + /// <returns> + /// true if the specified value is equal to the current value; otherwise, false. + /// For numeric values, true is equal to 1 and false is equal to 0. + /// </returns> public override bool Equals(BslValue other)src/Tests/OneScript.Core.Tests/ValuesTest.cs (1)
204-233: Comprehensive equality testing implementation.The test method thoroughly verifies all combinations of boolean and numeric value comparisons, including edge cases.
Consider adding test cases for:
- Comparison with undefined values
- Comparison with null values
- Comparison with string representations ("true"/"false")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/OneScript.Core/Values/BslBooleanValue.cs(2 hunks)src/OneScript.StandardLibrary/Collections/ValueList/ValueListImpl.cs(1 hunks)src/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cs(2 hunks)src/OneScript.StandardLibrary/Collections/ValueTree/ValueTreeRowCollection.cs(2 hunks)src/Tests/OneScript.Core.Tests/ValuesTest.cs(1 hunks)tests/find-in-collections.os(1 hunks)
🔇 Additional comments (11)
src/OneScript.Core/Values/BslBooleanValue.cs (1)
12-12: LGTM!The added using statement is required for the changes in the Equals method.
tests/find-in-collections.os (5)
1-27: Well-organized test structure!The test setup is comprehensive and follows a clear pattern, grouping related tests by collection type.
29-47: Thorough array testing implementation.Tests cover both directions of type conversion (boolean-to-number and number-to-boolean) and both true/false values.
73-91: Comprehensive value list testing.Tests verify that strict comparison prevents false positives between boolean and numeric values.
94-124: Complete value table testing coverage.Tests include both direct value search and row filtering scenarios.
159-189: Thorough value tree testing implementation.Tests cover both direct value search and row filtering in tree structures.
src/OneScript.StandardLibrary/Collections/ValueList/ValueListImpl.cs (1)
185-185: LGTM! Improved value comparison.The change from
EqualstoStrictEqualsaligns with the PR objective of implementing strict value comparison.src/OneScript.StandardLibrary/Collections/ValueTree/ValueTreeRowCollection.cs (2)
251-251: LGTM! The change to strict comparison aligns with the PR objective.The change from
EqualstoStrictEqualsensures more precise value matching in the filter criteria validation.Since this change affects filter behavior, let's verify if there are any test cases that rely on loose comparison in filters:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Search for test cases that might be affected by the strict comparison change in filters. # Test: Search for test files containing filter-related tests rg -A 5 "test.*Filter.*" "*test*"Length of output: 82
I'll generate a new shell script to list any C# test files (which typically include "Test" in their filename) so we can check if there are relevant tests that depend on the filter behavior.
🏁 Script executed:
#!/bin/bash # List all C# files with "Test" (case-insensitive) in their filenames, # which are likely to be test files in the repository. fd -e cs -i "test"Length of output: 2153
LGTM – Strict equality update confirmed.
The change from
EqualstoStrictEqualsin the filter criteria check is in line with the PR’s objective of enforcing a more precise value comparison. Our investigation into the test suite (listing C# files with "Test" in their names) didn’t reveal any tests that depend on loose equality. While we didn’t locate filter-specific tests directly, there are no indications that this change will break existing behavior.
227-227: LGTM! The change to strict comparison aligns with the PR objective.The change from
EqualstoStrictEqualsensures more precise value matching in the tree search functionality.Since this change affects value comparison behavior, let's verify if there are any test cases that rely on loose comparison:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Search for test cases that might be affected by the strict comparison change. # Test: Search for test files containing "Find" method tests rg -A 5 "test.*Find.*" "*test*"Length of output: 80
I'll generate a new shell script to search for potential C# test files (using common naming conventions) and look for references to the
Findmethod within them. This should help us verify whether there are any test cases affected by the changed comparison behavior.Please run the above script so we can gather the necessary information for further verification.
🏁 Script executed:
#!/bin/bash # Listing C# test files using common naming conventions (e.g., files containing "Test" or "Tests" in the filename) echo "Listing C# test files based on naming convention:" fd -e cs --regex ".*(Test|Tests)\\.cs$" echo "" echo "Searching for references to the 'Find' method invocation in these test files:" fd -e cs --regex ".*(Test|Tests)\\.cs$" --exec grep -Hn "Find(" {}Length of output: 2012
Change Approved: Strict Value Comparison Verified
Our investigation did not reveal any test cases relying on loose equality for the
Findmethod. The new use ofStrictEqualsappears consistent with similar changes in the codebase, and our search of test files did not identify any tests that would be adversely affected by this behavior change.src/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cs (2)
277-277: LGTM! The change to strict comparison aligns with the PR objective.The change from
EqualstoStrictEqualsensures more precise value matching in the filter criteria validation.Since this change affects filter behavior in tables, let's verify if there are any test cases that rely on loose comparison in filters:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Search for test cases that might be affected by the strict comparison change in table filters. # Test: Search for test files containing table filter-related tests rg -A 5 "test.*Table.*Filter.*" "*test*"Length of output: 89
I'm not seeing any tests with the previous search command. To make sure we capture any test cases that might be affected by the stricter filter, please run the following shell script to search for C# test files and then search within them for keywords like
"ValueTable","CheckFilterCriteria", and"StrictEquals":Please run the script and share the output so we can further verify if there are any affected tests.
🏁 Script executed:
#!/bin/bash echo "Listing potential C# test files (filename contains 'test'):" fd --extension cs -i "test" echo "Searching for 'ValueTable' in potential test files:" fd --extension cs -i "test" | xargs -I {} grep -Hn "ValueTable" {} echo "Searching for 'CheckFilterCriteria' in the repository:" rg -Hn "CheckFilterCriteria" -- "*.cs" echo "Searching for 'StrictEquals' in the repository:" rg -Hn "StrictEquals" -- "*.cs"Length of output: 3772
LGTM. The change to use
StrictEqualsfor filter matching is consistent with the PR objective and does not appear to impact existing tests.Based on the repository search:
- No explicit test reference was found that depends on the previous loose (
Equals) behavior.- Tests referencing
ValueTable(e.g., in NUnit and OneScript.Core.Tests) remain unaffected by this change.
261-261: LGTM! The change to strict comparison aligns with the PR objective.The change from
EqualstoStrictEqualsensures more precise value matching in the table search functionality.Since this change affects value comparison behavior in tables, let's verify if there are any test cases that rely on loose comparison:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Search for test cases that might be affected by the strict comparison change in tables. # Test: Search for test files containing table-related tests rg -A 5 "test.*Table.*Find.*" "*test*"Length of output: 87
I'll run another round of verification to see if there are any test files that reference the table search functionality—particularly any that might be affected by the switch to using
StrictEquals.
🏁 Script executed:
#!/bin/bash # Searching for test files that mention "ValueTable" since these tests may reference the Find method. fd -t f -i "test" | xargs -r rg "ValueTable" # Additionally, looking in test files for any usage of "StrictEquals" which might indicate reliance on strict comparison. fd -t f -i "test" | xargs -r rg --ignore-case "StrictEquals"Length of output: 962
I'll run another verification script to search for the definition and usage of
StrictEqualsin the repository. This will help ensure that the new strict comparison is implemented consistently and that no tests rely on the old loose comparison.
🏁 Script executed:
#!/bin/bash # Search the entire repository for occurrences of "StrictEquals" to verify its definition and usage. rg -w StrictEqualsLength of output: 901
LGTM! The consistent use of
StrictEqualsacross the codebase confirms the intent for stricter comparisons, and there are no tests that rely on the previous loose equality behavior.
2 participants
Исправление, минимально затрагивающее исходники.
TODO: найти все места, требующие строгого сравнения
Summary by CodeRabbit
New Features
Refactor