fix #1577: сортировка неупорядочиваемых типов +тест - #1578
Conversation
Sorry, something went wrong.
WalkthroughIntroduces process-aware value comparison: GenericIValueComparer gains constructors and swaps its equality shortcut to ReferenceEquals(x,y); ValueTable.Sort now accepts an IBslProcess and passes it into sorting comparers; tests were added validating sorting of non-orderable, mixed types, and presentation-based ordering. Changes
Sequence Diagram(s)sequenceDiagram
participant VT as ValueTable.Sort
participant RC as RowComparator
participant GVC as GenericIValueComparer
participant P as IBslProcess
rect rgba(60,179,113,0.08)
VT->>RC: new RowComparator(process, rules)
RC->>GVC: new GenericIValueComparer(process)
end
rect rgba(70,130,180,0.06)
Note over GVC: comparer chosen in ctor\nCompareByPresentations when process provided\nelse CompareAsStrings
end
loop for each pair to compare
RC->>GVC: Compare(x, y)
alt same reference
GVC-->>RC: 0
else if both comparable and same SystemType
GVC-->>RC: x.CompareTo(y)
else
GVC-->>RC: _comparer(x,y) -- string or presentation compare
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem✨ Finishing Touches
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. ❤️ Share 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type @coderabbitai help to get the list of available commands. Other keywords and placeholders
CodeRabbit Configuration File (.coderabbit.yaml)
Status, Documentation and Community
|
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)src/ScriptEngine/Machine/GenericIValueComparer.cs (2)📜 Review detailstests/valuetable.os (2)42-49: Add null-guards to avoid potential NRE if a caller ever passes C# null.
Even if current call sites never pass null, a defensive check is cheap and prevents fragile failures.
Apply within this hunk:
public int Compare(IValue x, IValue y) { - if (ReferenceEquals(x, y)) + if (ReferenceEquals(x, y)) return 0; + if (ReferenceEquals(x, null)) return -1; + if (ReferenceEquals(y, null)) return 1; if (x.SystemType == y.SystemType) return x.CompareTo(y); else return x.ToString().CompareTo(y.ToString()); }Optionally mirror similar guards in Equals(IValue x, IValue y) and GetHashCode(IValue obj) (return 0 for obj == null).
45-49: Make cross-type string fallback deterministic (optional).
string.CompareTo is culture-sensitive; if you need stable ordering across locales/runs, prefer an explicit comparer.
Example:
// add: using System; return StringComparer.CurrentCulture.Compare(x.ToString(), y.ToString()); // or, if strict binary order is desirable: // return StringComparer.Ordinal.Compare(x.ToString(), y.ToString());786-794: Strengthen the test with basic post-conditions.
Assert row count and value preservation to ensure no regressions beyond “no exception thrown”.
Apply:
ТЗ.Сортировать("Тест"); + юТест.ПроверитьРавенство(ТЗ.Количество(), 2); + юТест.ПроверитьРавенство(ТЗ[0].Тест, Неопределено); + юТест.ПроверитьРавенство(ТЗ[1].Тест, Неопределено); КонецПроцедуры
796-806: Verify invariants after sorting mixed, non-orderable types.
Add minimal checks that all inserted values remain present and count unchanged.
Apply:
ТЗ.Сортировать("Тест"); + юТест.ПроверитьРавенство(ТЗ.Количество(), 4); + // Presence checks (order is unspecified) + юТест.ПроверитьЛожь(ТЗ.Найти(Тип("Строка"), "Тест") = Неопределено); + юТест.ПроверитьЛожь(ТЗ.Найти(Неопределено, "Тест") = Неопределено); + юТест.ПроверитьЛожь(ТЗ.Найти(null, "Тест") = Неопределено); КонецПроцедуры
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📥 CommitsReviewing files that changed from the base of the PR and between 3581fdd and 971d555.
📒 Files selected for processing (2)- src/ScriptEngine/Machine/GenericIValueComparer.cs (1 hunks)
- tests/valuetable.os (2 hunks)
src/ScriptEngine/Machine/GenericIValueComparer.cs (1)tests/valuetable.os (1)42-45: Identity short-circuit is correct and fixes sorting for singleton non-orderables.
if (ReferenceEquals(x, y)) return 0; is a good early-exit and will prevent accidental calls into CompareTo for singleton values like Неопределено/NULL during sort.
46-47: Good: tests are registered in the suite.
Sorry, something went wrong.
|
По замечаниям кролика:
про сравнение строковых представлений:
|
Sorry, something went wrong.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)tests/valuetable.os (1)📜 Review detailssrc/ScriptEngine/Machine/GenericIValueComparer.cs (1)786-799: Undefined values sort smoke-test is fine.
Covers the “should not crash” case for non-orderable primitives. Consider adding one mixed-case (Неопределено вместе с другим типом) to lock ordering vs others, if that’s important.
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cs (2)65-74: Early-exit is correct; also add symmetric IComparable check and null-guard.
- ReferenceEquals(x, y) is the right fix (answering the “|| vs &&” question: OR would be wrong because it would treat null vs non-null as equal).
- Make the orderable path symmetric and resilient; fall back to presentation/string compare otherwise.
Suggested tightening:
public int Compare(IValue x, IValue y) { if (ReferenceEquals(x, y)) return 0; - - if (x is IComparable && x.SystemType == y.SystemType ) - return x.CompareTo(y); - else - return _comparer(x,y); + // Defensive null-ordering (shouldn't happen via ValueTable, but safe in generic comparer) + if (x is null) return y is null ? 0 : -1; + if (y is null) return 1; + + if (x.SystemType == y.SystemType && x is IComparable && y is IComparable) + return x.CompareTo(y); + + return _comparer(x, y); }682-685: comparator parameter is still ignored.
If custom comparison rules are intended via “СравнениеЗначений”, wire them through RowComparator; otherwise consider removing the parameter to avoid API confusion.
682-685: Align sorting behavior across collections.
ValueTree currently uses GenericIValueComparer() without IBslProcess (see ValueTreeRowCollection.RowComparator in the repo). For consistency, consider passing IBslProcess there as well to keep presentation-based ordering uniform across Table and Tree.
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📥 CommitsReviewing files that changed from the base of the PR and between 971d555 and 15235dc.
📒 Files selected for processing (3)- src/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cs (3 hunks)
- src/ScriptEngine/Machine/GenericIValueComparer.cs (2 hunks)
- tests/valuetable.os (2 hunks)
src/ScriptEngine/Machine/GenericIValueComparer.cs (3)src/ScriptEngine/Machine/GenericIValueComparer.cs (2)src/OneScript.StandardLibrary/Collections/ValueTree/ValueTreeRowCollection.cs (5)
- GenericIValueComparer (16-75)
- GenericIValueComparer (21-24)
- GenericIValueComparer (26-30)
- RowComparator (388-423)
- RowComparator (394-400)
- List (354-386)
- Sort (438-449)
- IValue (486-489)
src/OneScript.Core/Values/BslValue.cs (6)🔇 Additional comments (5)src/ScriptEngine/ScriptingEngine.cs (1)
- IValue (77-77)
- ToString (25-28)
- CompareTo (30-30)
- CompareTo (73-73)
- BslValue (17-88)
- BslValue (79-85)
- IBslProcess (206-206)
tests/valuetable.os (3)src/ScriptEngine/Machine/GenericIValueComparer.cs (1)46-49: Added tests registered — good coverage bump.
New tests are correctly appended to the suite.
800-820: Locale-dependent expectations — please confirm CI culture or relax assertions.
This test asserts a specific sequence that can change with system collation. Either:
- fix the test harness culture (e.g., ru-RU), or
- assert that AsString(process) produces a non-decreasing sequence instead of exact positions.
821-858: Presentation-based sorting test is on-point.
Validates ToString(process)/ОбработкаПолученияПредставления usage through Sort — nice.
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cs (1)21-31: Constructor-based strategy selection is clean.
Default → string compare; with process → presentation compare. Clear and extensible.
639-648: Comparer now process-aware — good integration.
RowComparator builds GenericIValueComparer(process) once per sort; avoids per-compare allocations and unlocks presentation-aware ordering.
Sorry, something went wrong.
| private int CompareAsStrings(IValue x, IValue y) | ||
| { | ||
| return x.ToString().CompareTo(y.ToString()); | ||
| } | ||
|
|
||
| private int CompareByPresentations(IValue x, IValue y) | ||
| { | ||
| return ((BslValue)x).ToString(_process).CompareTo(((BslValue)y).ToString(_process)); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid invalid casts; use AsString(process) to compare presentations.
Casting to BslValue may blow up for IValueReference and similar wrappers. Use the safe API that already unwraps values and respects process context.
Apply:
- private int CompareByPresentations(IValue x, IValue y)
- {
- return ((BslValue)x).ToString(_process).CompareTo(((BslValue)y).ToString(_process));
- }
+ private int CompareByPresentations(IValue x, IValue y)
+ {
+ return x.AsString(_process).CompareTo(y.AsString(_process));
+ }‼️ 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.
| private int CompareAsStrings(IValue x, IValue y) | |
| { | |
| return x.ToString().CompareTo(y.ToString()); | |
| } | |
| private int CompareByPresentations(IValue x, IValue y) | |
| { | |
| return ((BslValue)x).ToString(_process).CompareTo(((BslValue)y).ToString(_process)); | |
| } | |
| private int CompareAsStrings(IValue x, IValue y) | |
| { | |
| return x.ToString().CompareTo(y.ToString()); | |
| } | |
| private int CompareByPresentations(IValue x, IValue y) | |
| { | |
| return x.AsString(_process).CompareTo(y.AsString(_process)); | |
| } |
In src/ScriptEngine/Machine/GenericIValueComparer.cs around lines 55 to 64, the CompareByPresentations method incorrectly casts IValue to BslValue which can fail for IValueReference or other wrappers; replace the cast-and-ToString(_process) calls with the safe AsString(_process) API on the IValue instances (e.g. x.AsString(_process) and y.AsString(_process)) so values are unwrapped and presentation respects process context, then compare those strings.
Sorry, something went wrong.
There was a problem hiding this comment.
- AsString - это адаптер к вызову ((BslValue)val).ToString(process)
- IValueReference не наследуется от IValue
- Прочие объекты являются потомками BslValue, кастинг безопасен
Sorry, something went wrong.
There was a problem hiding this comment.
@Mr-Rm, thank you for the clarification about the OneScript type system. I appreciate you correcting my understanding:
- If AsString is indeed just an adapter to ((BslValue)val).ToString(process), then my suggestion doesn't provide any additional safety benefit.
- You're right that IValueReference not inheriting from IValue means it can't be passed to this comparer method.
- With all IValue implementations being descendants of BslValue, the cast is indeed type-safe.
I withdraw my suggestion - it was based on incorrect assumptions about the type hierarchy. The current implementation is appropriate given the actual design of the OneScript value system.
✏️ Learnings added
Learnt from: Mr-Rm PR: EvilBeaver/OneScript#1578 File: src/ScriptEngine/Machine/GenericIValueComparer.cs:55-64 Timestamp: 2025-09-04T11:15:14.287Z Learning: In OneScript, `AsString(_process)` is just an adapter that internally calls `((BslValue)val).ToString(process)`, so using the cast directly is equivalent.
Learnt from: Mr-Rm PR: EvilBeaver/OneScript#1578 File: src/ScriptEngine/Machine/GenericIValueComparer.cs:55-64 Timestamp: 2025-09-04T11:15:14.287Z Learning: In OneScript, `IValueReference` does not inherit from `IValue`, so it cannot be passed to methods expecting `IValue` parameters.
Learnt from: Mr-Rm PR: EvilBeaver/OneScript#1578 File: src/ScriptEngine/Machine/GenericIValueComparer.cs:55-64 Timestamp: 2025-09-04T11:15:14.287Z Learning: In OneScript, all objects that implement `IValue` are descendants of `BslValue`, making casting from `IValue` to `BslValue` type-safe.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: Mr-Rm PR: EvilBeaver/OneScript#1575 File: src/OneScript.Core/Values/BslDateValue.cs:48-48 Timestamp: 2025-08-25T07:41:13.053Z Learning: BslObjectValue and BslPrimitiveValue in OneScript are abstract base classes and should not implement concrete GetHashCode() methods.
Sorry, something went wrong.
Мне кажется, не стоит. Это долго Вижу, что уже сделано, ок |
Sorry, something went wrong.
Отрабатывает примитивы Неопределено и NULL, также оптимизирует сравнение совпадающих строк и других объектов.
Summary by CodeRabbit
Bug Fixes
Changes
Tests