fix #1582: сравнение примитивных типов - #1584
Conversation
Sorry, something went wrong.
WalkthroughCentralizes comparison by making BslValue.CompareTo virtual with a default ComparisonException path; adds IBslComparable and implements it on primitive value types; updates GenericIValueComparer with type ordering and IBslComparable checks; removes many per-type CompareTo overrides; introduces ColumnException and refactors collection error handling; expands sorting tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tests
participant Sort as ValueTable.Sort
participant Cmp as GenericIValueComparer
participant A as Value A (IValue)
participant B as Value B (IValue)
note right of Cmp `#e7f3ff`: Gate: IBslComparable + SystemType ordering
Tests->>Sort: request Sort(column(s))
Sort->>Cmp: Compare(A, B)
alt A implements IBslComparable AND A.SystemType == B.SystemType
Cmp->>A: CompareTo(B)
A-->>Cmp: int result
else if System types differ
Cmp->>Cmp: CompareByTypes(A, B) (orderedTypes / INDEX_OF_TYPE)
Cmp-->>Sort: int result
else
Cmp->>Cmp: FallbackCompareByPresentation(A, B)
Cmp-->>Sort: int result
end
Sort-->>Tests: sorted rows
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas to check closely:
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
📜 Recent review details Configuration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📥 CommitsReviewing files that changed from the base of the PR and between ea739f8 and 453425d. 📒 Files selected for processing (1)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)src/OneScript.Core/Values/IBslComparable.cs (1)📜 Review detailssrc/ScriptEngine/Machine/GenericIValueComparer.cs (1)10-12: Add a brief doc comment to clarify the marker’s intent
Helps future readers understand why this empty interface exists and how it’s used by the comparer.
namespace OneScript.Values { - public interface IBslComparable + /// <summary> + /// Marker interface for BSL values whose CompareTo provides a meaningful total order + /// within the same SystemType. Used by GenericIValueComparer to route typed comparisons. + /// </summary> + public interface IBslComparable { } }70-71: Tighten the typed-compare guard to be symmetric
Minor robustness: also check y implements the marker before CompareTo.
- if (x is IBslComparable && x.SystemType == y.SystemType ) + if (x is IBslComparable && y is IBslComparable && x.SystemType == y.SystemType) return x.CompareTo(y);
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 3845405 and b3e8482.
📒 Files selected for processing (7)- src/OneScript.Core/Values/BslBooleanValue.cs (1 hunks)
- src/OneScript.Core/Values/BslDateValue.cs (1 hunks)
- src/OneScript.Core/Values/BslNumericValue.cs (1 hunks)
- src/OneScript.Core/Values/BslStringValue.cs (1 hunks)
- src/OneScript.Core/Values/IBslComparable.cs (1 hunks)
- src/ScriptEngine/Machine/GenericIValueComparer.cs (1 hunks)
- tests/valuetable.os (2 hunks)
Learnt from: Mr-Rm PR: EvilBeaver/OneScript#1578 File: src/ScriptEngine/Machine/GenericIValueComparer.cs:55-64 Timestamp: 2025-09-04T11:15:14.305Z Learning: In OneScript, all objects that implement `IValue` are descendants of `BslValue`, making casting from `IValue` to `BslValue` type-safe.
Applied to files:
- src/OneScript.Core/Values/BslNumericValue.cs
- src/OneScript.Core/Values/BslStringValue.cs
- src/OneScript.Core/Values/BslDateValue.cs
- src/OneScript.Core/Values/IBslComparable.cs
- src/OneScript.Core/Values/BslBooleanValue.cs
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.
Applied to files:
- src/OneScript.Core/Values/BslNumericValue.cs
- src/OneScript.Core/Values/BslStringValue.cs
- src/OneScript.Core/Values/BslDateValue.cs
- src/OneScript.Core/Values/IBslComparable.cs
- src/OneScript.Core/Values/BslBooleanValue.cs
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: BslNullValue, BslSkippedParameterValue, and BslUndefinedValue in OneScript use singleton patterns with static Instance properties and ReferenceEquals-based equality, so they should not override GetHashCode().
Applied to files:
- src/OneScript.Core/Values/BslNumericValue.cs
- src/OneScript.Core/Values/BslStringValue.cs
- src/OneScript.Core/Values/BslBooleanValue.cs
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: BslBooleanValue in OneScript uses a flyweight pattern with static True/False instances and reference-based equality, so it should not override GetHashCode().
Applied to files:
- src/OneScript.Core/Values/BslNumericValue.cs
- src/OneScript.Core/Values/BslStringValue.cs
- src/OneScript.Core/Values/IBslComparable.cs
- src/OneScript.Core/Values/BslBooleanValue.cs
src/OneScript.Native/Compiler/CompilerHelpers.cs (1)src/OneScript.Core/Values/BslStringValue.cs (2)src/OneScript.Core/Values/BslPrimitiveValue.cs (1)
- BslPrimitiveValue (22-34)
- BslPrimitiveValue (13-38)
src/OneScript.Native/Compiler/CompilerHelpers.cs (1)src/OneScript.Core/Values/BslDateValue.cs (1)src/OneScript.Core/Values/BslPrimitiveValue.cs (1)
- BslPrimitiveValue (22-34)
- BslPrimitiveValue (13-38)
src/OneScript.Native/Compiler/CompilerHelpers.cs (1)src/OneScript.Core/Values/BslBooleanValue.cs (2)
- BslPrimitiveValue (22-34)
src/OneScript.Native/Compiler/CompilerHelpers.cs (1)🔇 Additional comments (9)src/OneScript.Core/Values/BslPrimitiveValue.cs (1)
- BslPrimitiveValue (22-34)
- BslPrimitiveValue (13-38)
src/OneScript.Core/Values/BslStringValue.cs (1)src/OneScript.Core/Values/BslDateValue.cs (1)13-13: IBslComparable on strings — looks good
The marker interface addition aligns with the new comparer flow; no logic changes needed here.
src/OneScript.Core/Values/BslBooleanValue.cs (1)15-15: IBslComparable on dates — consistent with design
Date CompareTo/Equals already implement the semantics; the marker cleanly enables the fast path.
src/OneScript.Core/Values/BslNumericValue.cs (1)15-15: IBslComparable on booleans — OK
Matches numeric/boolean ordering behavior used in tests; no extra changes required.
tests/valuetable.os (5)17-17: IBslComparable on numbers — good addition
Enables numeric ordering via the new comparer path; existing arithmetic/equality remain intact.
46-49: Registering new sort tests — looks right
All four tests are properly added to the suite.
791-804: Number sort test — passes intent clearly
Covers negatives and positives; assertions are precise.
806-819: Boolean sort test — validates 0/1 ordering
Confirms False < True as expected.
821-839: Date sort test — OK
Checks relative dates and a minimal value; ordering matches CompareTo semantics.
840-855: String sort test — culture-aware scenario covered
Good to see ё/е and case interactions exercised.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)src/OneScript.StandardLibrary/GuidWrapper.cs (1)src/OneScript.Core/Values/BslPrimitiveValue.cs (1)61-67: Possible NullReferenceException when other is null; align with base semantics.
other.GetRawValue() dereferences other without a null check. In BslPrimitiveValue.CompareTo, other == null returns -1; this override should not NRE.
Apply:
public override int CompareTo(BslValue other) { - GuidWrapper otherUuid = other.GetRawValue() as GuidWrapper; + if (other == null) + return -1; + + GuidWrapper otherUuid = other.GetRawValue() as GuidWrapper; if (otherUuid == null) - throw ComparisonException.NotSupported(); + throw ComparisonException.NotSupported(); return _value.CompareTo(otherUuid._value); }30-32: Bug: wrong type captured for “other” in exception context.
typeOfOther is set from this.GetType() instead of other.GetType().
Apply:
- typeOfOther ??= GetType().ToString(); + typeOfOther ??= other.GetType().ToString();
src/ScriptEngine/Machine/Contexts/ContextIValueImpl.cs (1)📜 Review detailssrc/OneScript.Core/Values/EnumerationValue.cs (1)63-66: Switch to ComparisonException looks right; consider adding type for better diagnostics.
Using the typed overload can improve error messages during debugging.
Apply:
- throw ComparisonException.NotSupported(); + throw ComparisonException.NotSupported(SystemType.Name);src/OneScript.Core/Values/BslObjectValue.cs (1)49-52: Consistent exception type; consider including the enum type name.
Optional: pass SystemType.Name to enrich the message.
Apply:
- throw ComparisonException.NotSupported(); + throw ComparisonException.NotSupported(SystemType.Name);src/OneScript.Core/Exceptions/ComparisonException.cs (1)15-18: Consistent exception; consider typed overload for clarity.
Including SystemType.Name can help users pinpoint the failing type.
Apply:
- throw ComparisonException.NotSupported(); + throw ComparisonException.NotSupported(SystemType.Name);33-38: Trailing space in RU message.
There’s an extra space at the end of the Russian string.
Apply:
- $"Сравнение на больше/меньше типов '{type1}' и '{type2}' не поддерживается ", + $"Сравнение на больше/меньше типов '{type1}' и '{type2}' не поддерживается",
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between b3e8482 and 44a55bd.
📒 Files selected for processing (8)- src/OneScript.Core/Exceptions/ComparisonException.cs (1 hunks)
- src/OneScript.Core/Exceptions/RuntimeException.cs (0 hunks)
- src/OneScript.Core/Values/BslObjectValue.cs (1 hunks)
- src/OneScript.Core/Values/BslPrimitiveValue.cs (1 hunks)
- src/OneScript.Core/Values/EnumerationValue.cs (1 hunks)
- src/OneScript.Core/Values/IBslComparable.cs (1 hunks)
- src/OneScript.StandardLibrary/GuidWrapper.cs (1 hunks)
- src/ScriptEngine/Machine/Contexts/ContextIValueImpl.cs (1 hunks)
- src/OneScript.Core/Exceptions/RuntimeException.cs
- src/OneScript.Core/Values/IBslComparable.cs
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.
Applied to files:
- src/OneScript.Core/Values/BslObjectValue.cs
- src/OneScript.Core/Values/EnumerationValue.cs
src/OneScript.Core/Exceptions/ComparisonException.cs (5)src/OneScript.Core/Values/BslObjectValue.cs (1)
- ComparisonException (13-39)
- ComparisonException (15-17)
- ComparisonException (19-24)
- ComparisonException (26-31)
- ComparisonException (33-38)
src/OneScript.Core/Exceptions/ComparisonException.cs (5)src/ScriptEngine/Machine/Contexts/ContextIValueImpl.cs (1)
- ComparisonException (13-39)
- ComparisonException (15-17)
- ComparisonException (19-24)
- ComparisonException (26-31)
- ComparisonException (33-38)
src/OneScript.Core/Exceptions/ComparisonException.cs (5)src/OneScript.StandardLibrary/GuidWrapper.cs (1)
- ComparisonException (13-39)
- ComparisonException (15-17)
- ComparisonException (19-24)
- ComparisonException (26-31)
- ComparisonException (33-38)
src/OneScript.Core/Exceptions/ComparisonException.cs (5)src/OneScript.Core/Exceptions/ComparisonException.cs (1)
- ComparisonException (13-39)
- ComparisonException (15-17)
- ComparisonException (19-24)
- ComparisonException (26-31)
- ComparisonException (33-38)
src/OneScript.Core/Exceptions/RuntimeException.cs (16)src/OneScript.Core/Values/EnumerationValue.cs (1)
- RuntimeException (13-174)
- RuntimeException (15-17)
- RuntimeException (19-21)
- RuntimeException (23-26)
- RuntimeException (30-35)
- RuntimeException (37-42)
- RuntimeException (44-49)
- RuntimeException (51-56)
- RuntimeException (58-63)
- RuntimeException (65-70)
- RuntimeException (72-77)
- RuntimeException (79-84)
- RuntimeException (86-91)
- RuntimeException (93-98)
- RuntimeException (100-105)
- RuntimeException (107-112)
src/OneScript.Core/Exceptions/ComparisonException.cs (5)🔇 Additional comments (3)
- ComparisonException (13-39)
- ComparisonException (15-17)
- ComparisonException (19-24)
- ComparisonException (26-31)
- ComparisonException (33-38)
src/OneScript.Core/Values/BslPrimitiveValue.cs (1)src/OneScript.Core/Exceptions/ComparisonException.cs (2)34-35: Good: unified to ComparisonException with both type names.
Consistent with the new centralized exception.
13-39: Centralized comparison exception: LGTM.
Clear API with RU/EN messages and useful overloads.
13-39: Update remaining 'ComparisonNotSupportedException' reference in tests.
Leftover TODO at src/Tests/OneScript.Core.Tests/ValuesTest.cs:190 references ComparisonNotSupportedException — change it to ComparisonException (or remove the TODO). No other code references found; CompareTo implementations call ComparisonException.NotSupported().
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)tests/engine-behaviors.os (1)📜 Review details686-690: Align expected error text with new ComparisonException.NotSupported(type).
Switching to a type-specific message for comparing values of script type 'Тип' matches the new exception contract. Looks good.
- Consider using NStr for bilingual stability, similar to other tests.
- Minor: rename variable to ОшибкаДляТипа for grammatical consistency.
- Add one more assertion to cover the two-type variant (e.g., Число vs Строка) to exercise NotSupported(type1, type2).
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 44a55bd and ed060af.
📒 Files selected for processing (3)- src/OneScript.Core/Exceptions/ComparisonException.cs (1 hunks)
- src/OneScript.Core/Values/BslPrimitiveValue.cs (1 hunks)
- tests/engine-behaviors.os (1 hunks)
- src/OneScript.Core/Exceptions/ComparisonException.cs
src/OneScript.Core/Exceptions/ComparisonException.cs (5)
- ComparisonException (13-39)
- ComparisonException (15-17)
- ComparisonException (19-24)
- ComparisonException (26-31)
- ComparisonException (33-38)
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)src/OneScript.Core/Values/BslPrimitiveValue.cs (1)🧹 Nitpick comments (2)30-31: Fix applied: correct fallback for other operand — LGTM
typeOfOther now uses other.GetType(). Matches the earlier review note.
src/OneScript.Core/Values/BslPrimitiveValue.cs (2)📜 Review details30-31: Nit: prefer Type.Name over ToString() for consistency
Aligns with SystemType.Name above and avoids namespace/fullname mismatches.
- typeOfThis ??= this.GetType().ToString(); - typeOfOther ??= other.GetType().ToString(); + typeOfThis ??= GetType().Name; + typeOfOther ??= other.GetType().Name;
34-37: Avoid string-based equality; compare types to choose exception overload
Prevents false inequality when one side comes from SystemType.Name and the other from CLR Type.
- if (typeOfThis == typeOfOther) - throw ComparisonException.NotSupported(typeOfThis); - else - throw ComparisonException.NotSupported(typeOfThis, typeOfOther); + var sameType = + (typeOfThis != null && typeOfOther != null && typeOfThis == typeOfOther) + || ((typeOfThis == null || typeOfOther == null) && GetType() == other.GetType()); + if (sameType) + throw ComparisonException.NotSupported(typeOfThis ?? GetType().Name); + else + throw ComparisonException.NotSupported(typeOfThis ?? GetType().Name, + typeOfOther ?? other.GetType().Name);
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between ed060af and 8d71eee.
📒 Files selected for processing (1)- src/OneScript.Core/Values/BslPrimitiveValue.cs (1 hunks)
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.
Applied to files:
- src/OneScript.Core/Values/BslPrimitiveValue.cs
src/OneScript.Core/Exceptions/ComparisonException.cs (5)
- ComparisonException (13-39)
- ComparisonException (15-17)
- ComparisonException (19-24)
- ComparisonException (26-31)
- ComparisonException (33-38)
Sorry, something went wrong.
|
Сомнительная проверка и результат: OneScript/src/OneScript.Core/Values/BslPrimitiveValue.cs Lines 15 to 18 in 3845405 |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)src/Tests/OneScript.Core.Tests/ValuesTest.cs (1)📜 Review detailssrc/OneScript.Core/Exceptions/ComparisonException.cs (1)180-181: Use Assert.Throws instead of ThrowsAny for precision.
CompareTo now throws a specific ComparisonException. Prefer Assert.Throws<ComparisonException> to avoid masking other exception types.
- Assert.ThrowsAny<ComparisonException>(() => num1.CompareTo(num2)); - Assert.ThrowsAny<ComparisonException>(() => num2.CompareTo(num1)); + Assert.Throws<ComparisonException>(() => num1.CompareTo(num2)); + Assert.Throws<ComparisonException>(() => num2.CompareTo(num1));- Assert.ThrowsAny<ComparisonException>(() => v1.CompareTo(v2)); + Assert.Throws<ComparisonException>(() => v1.CompareTo(v2));Also applies to: 190-190
src/OneScript.Core/Values/BslValue.cs (1)13-18: Mark exception as serializable.
Custom exceptions should be serializable. Add the attribute to align with .NET guidelines (base types already reference Exception).
- public class ComparisonException : RuntimeException + [Serializable] + public class ComparisonException : RuntimeExceptiontests/engine-behaviors.os (2)35-47: De-duplicate type-name retrieval.
Use a helper to avoid try/catch duplication and tighten readability.
- string typeOfThis = null; - string typeOfOther = null; - - try - { - typeOfThis = this.SystemType.Name; - typeOfOther = other.SystemType.Name; - } - catch (InvalidOperationException) // если тип не зарегистрирован - { - typeOfThis ??= this.GetType().ToString(); - typeOfOther ??= other.GetType().ToString(); - } + string typeOfThis = FriendlyTypeName(this); + string typeOfOther = FriendlyTypeName(other); + + static string FriendlyTypeName(BslValue v) + { + try { return v.SystemType.Name; } + catch (InvalidOperationException) { return v.GetType().ToString(); } + }704-705: Remove stray double semicolon.
Cosmetic, but easy to tidy.
- |Массив2 = Новый Массив(1);; + |Массив2 = Новый Массив(1);
669-707: Consider locale-agnostic expectations.
Hard-coded RU strings can be brittle on EN locales. Using НСтр(...) for expected messages would make tests resilient.
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 8d71eee and 52c170d.
📒 Files selected for processing (12)- src/OneScript.Core/Exceptions/ComparisonException.cs (1 hunks)
- src/OneScript.Core/Values/BslBooleanValue.cs (1 hunks)
- src/OneScript.Core/Values/BslDateValue.cs (1 hunks)
- src/OneScript.Core/Values/BslNumericValue.cs (1 hunks)
- src/OneScript.Core/Values/BslObjectValue.cs (0 hunks)
- src/OneScript.Core/Values/BslPrimitiveValue.cs (0 hunks)
- src/OneScript.Core/Values/BslStringValue.cs (2 hunks)
- src/OneScript.Core/Values/BslValue.cs (2 hunks)
- src/OneScript.Core/Values/EnumerationValue.cs (0 hunks)
- src/ScriptEngine/Machine/Contexts/ContextIValueImpl.cs (0 hunks)
- src/Tests/OneScript.Core.Tests/ValuesTest.cs (2 hunks)
- tests/engine-behaviors.os (1 hunks)
- src/OneScript.Core/Values/BslPrimitiveValue.cs
- src/OneScript.Core/Values/EnumerationValue.cs
- src/ScriptEngine/Machine/Contexts/ContextIValueImpl.cs
- src/OneScript.Core/Values/BslObjectValue.cs
- src/OneScript.Core/Values/BslDateValue.cs
- src/OneScript.Core/Values/BslBooleanValue.cs
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.
Applied to files:
- src/OneScript.Core/Values/BslValue.cs
- src/OneScript.Core/Values/BslStringValue.cs
- src/OneScript.Core/Values/BslNumericValue.cs
Learnt from: Mr-Rm PR: EvilBeaver/OneScript#1578 File: src/ScriptEngine/Machine/GenericIValueComparer.cs:55-64 Timestamp: 2025-09-04T11:15:14.305Z Learning: In OneScript, all objects that implement `IValue` are descendants of `BslValue`, making casting from `IValue` to `BslValue` type-safe.
Applied to files:
- src/OneScript.Core/Values/BslValue.cs
- src/OneScript.Core/Values/BslStringValue.cs
- src/OneScript.Core/Values/BslNumericValue.cs
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: BslBooleanValue in OneScript uses a flyweight pattern with static True/False instances and reference-based equality, so it should not override GetHashCode().
Applied to files:
- src/OneScript.Core/Values/BslStringValue.cs
- src/OneScript.Core/Values/BslNumericValue.cs
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: BslNullValue, BslSkippedParameterValue, and BslUndefinedValue in OneScript use singleton patterns with static Instance properties and ReferenceEquals-based equality, so they should not override GetHashCode().
Applied to files:
- src/OneScript.Core/Values/BslStringValue.cs
- src/OneScript.Core/Values/BslNumericValue.cs
src/OneScript.Core/Exceptions/ComparisonException.cs (5)src/OneScript.Core/Exceptions/ComparisonException.cs (1)src/OneScript.Core/Values/BslBooleanValue.cs (1)
- ComparisonException (13-39)
- ComparisonException (15-17)
- ComparisonException (19-24)
- ComparisonException (26-31)
- ComparisonException (33-38)
src/OneScript.Core/Values/BslDateValue.cs (1)
- CompareTo (70-78)
src/OneScript.Core/Values/BslNumericValue.cs (1)
- CompareTo (26-32)
src/OneScript.Core/Values/BslStringValue.cs (1)
- CompareTo (97-105)
src/OneScript.Core/Values/BslValue.cs (2)
- CompareTo (50-59)
src/OneScript.Core/Contexts/Variable.cs (2)
- CompareTo (30-53)
- CompareTo (96-96)
- CompareTo (69-72)
- CompareTo (129-132)
src/OneScript.Core/Exceptions/RuntimeException.cs (16)src/OneScript.Core/Values/BslValue.cs (8)
- RuntimeException (13-174)
- RuntimeException (15-17)
- RuntimeException (19-21)
- RuntimeException (23-26)
- RuntimeException (30-35)
- RuntimeException (37-42)
- RuntimeException (44-49)
- RuntimeException (51-56)
- RuntimeException (58-63)
- RuntimeException (65-70)
- RuntimeException (72-77)
- RuntimeException (79-84)
- RuntimeException (86-91)
- RuntimeException (93-98)
- RuntimeException (100-105)
- RuntimeException (107-112)
src/OneScript.Core/Values/BslBooleanValue.cs (3)src/OneScript.Core/Values/BslStringValue.cs (2)src/OneScript.Core/Values/BslDateValue.cs (2)
- CompareTo (70-78)
- BslValue (80-83)
- ToString (53-56)
src/OneScript.Core/Values/BslNumericValue.cs (2)
- CompareTo (26-32)
- ToString (53-56)
src/OneScript.Core/Values/BslStringValue.cs (2)
- CompareTo (97-105)
- ToString (73-76)
src/OneScript.StandardLibrary/GuidWrapper.cs (2)
- CompareTo (50-59)
- ToString (31-31)
src/OneScript.Core/Contexts/Variable.cs (3)
- CompareTo (61-68)
- ToString (56-59)
src/ScriptEngine/Machine/Contexts/ContextIValueImpl.cs (1)
- CompareTo (69-72)
- CompareTo (129-132)
- ToString (158-161)
src/OneScript.Core/Exceptions/ComparisonException.cs (5)
- ToString (41-44)
- ComparisonException (13-39)
- ComparisonException (15-17)
- ComparisonException (19-24)
- ComparisonException (26-31)
- ComparisonException (33-38)
src/OneScript.Core/Values/BslPrimitiveValue.cs (1)src/OneScript.Core/Values/BslNumericValue.cs (1)src/OneScript.Native/Compiler/CompilerHelpers.cs (1)
- BslPrimitiveValue (10-13)
- BslPrimitiveValue (22-34)
src/OneScript.Core/Values/BslPrimitiveValue.cs (1)🔇 Additional comments (6)
- BslPrimitiveValue (10-13)
src/OneScript.Core/Values/BslNumericValue.cs (1)src/OneScript.Core/Values/BslStringValue.cs (2)17-17: LGTM: Numeric now participates in the IBslComparable contract.
This aligns numeric comparisons with the new comparison model and GenericIValueComparer gating.
src/OneScript.Core/Values/BslValue.cs (3)13-13: LGTM: Add IBslComparable.
Consistent with other primitive types; enables safe participation in ordered comparisons.
52-53: LGTM: Short-circuit self-comparison.
Avoids extra work and guarantees 0 without hitting base logic.
32-34: Verify CompareTo(null) semantics.
Current logic treats any non-null value as less than null (returns -1). In .NET, x.CompareTo(null) should return > 0. Please confirm intended ordering; if unintentional, flip to 1.
- if (other is null) - return -1; + if (other is null) + return 1;
49-53: Good defaulting to ComparisonException for unsupported comparisons.
The type-aware messages will improve diagnostics and align tests.
102-108: Static UnwrapReference is appropriate.
Keeps CompareTo/Equals bridges concise; casting is type-safe per codebase guarantees.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)tests/engine-behaviors.os (2)🧹 Nitpick comments (1)54-54: Fix missing closing apostrophe in expected message.
The test roster entry has been correctly updated to match the renamed test procedure.
684-687: Fix missing closing apostrophe in expected message.
As noted in the past review, the second type name lacks the trailing single quote.
- Ошибка = "Сравнение на больше/меньше типов 'ПеречислениеХешФункция' и 'ПеречислениеСтатусСообщения' не поддерживается"; + Ошибка = "Сравнение на больше/меньше типов 'ПеречислениеХешФункция' и 'ПеречислениеСтатусСообщения'" не поддерживается";
tests/engine-behaviors.os (1)📜 Review details701-706: Fix syntax error in test code.
There's a double semicolon in the test code that should be corrected for consistency.
- |Массив2 = Новый Массив(1);; + |Массив2 = Новый Массив(1);
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 52c170d and c571c4d.
📒 Files selected for processing (1)- tests/engine-behaviors.os (2 hunks)
tests/engine-behaviors.os (6)667-667: LGTM: Test procedure renamed for clarity.
The procedure name change from ТестДолжен_ПроверитьСравнениеНаБольшеМеньшее() to ТестДолжен_ПроверитьОшибкиСравненияНаБольшеМеньше() better reflects the test's purpose of checking comparison errors rather than just comparison operations.
669-672: Verify error message format consistency.
The error message format has been improved to include the specific type name in single quotes, which provides better debugging information. The test correctly expects the new format: "Сравнение на больше/меньше для типа 'СтатусСообщения' не поддерживается".
674-677: LGTM: Enhanced error message for mixed enum types.
The test now checks for a more specific error message when comparing different enumeration types, which will help developers understand exactly which types are incompatible.
679-682: LGTM: Test for text encoding enumeration comparison.
The test properly validates that comparing text encoding enumerations throws the expected error with the correct type name format.
689-692: LGTM: Test for Type comparison error.
The test correctly validates that comparing Тип (Type) objects throws the expected error message format.
694-699: LGTM: Test for mixed collection types comparison.
The test properly checks that comparing different collection types (Array vs Structure) produces an appropriate error message indicating both types involved in the comparison.
Sorry, something went wrong.
EvilBeaver
left a comment
There was a problem hiding this comment.
Спасибо!
Sorry, something went wrong.
| namespace OneScript.Values | ||
| { | ||
| /// <summary> | ||
| /// ���������-������ ��� ����������� �����, ����������� ��������� �� ������/������. |
There was a problem hiding this comment.
Кодировка слетела
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review detailsConfiguration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between c571c4d and ea739f8.
📒 Files selected for processing (12)- src/OneScript.Core/Values/IBslComparable.cs (1 hunks)
- src/OneScript.StandardLibrary/Collections/Exceptions/ColumnException.cs (1 hunks)
- src/OneScript.StandardLibrary/Collections/ValueTable/CollectionIndexes.cs (2 hunks)
- src/OneScript.StandardLibrary/Collections/ValueTable/ColumnNotFoundException.cs (0 hunks)
- src/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cs (8 hunks)
- src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumn.cs (2 hunks)
- src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumnCollection.cs (3 hunks)
- src/OneScript.StandardLibrary/Collections/ValueTree/ValueTreeColumn.cs (2 hunks)
- src/OneScript.StandardLibrary/Collections/ValueTree/ValueTreeColumnCollection.cs (3 hunks)
- src/OneScript.StandardLibrary/Collections/ValueTree/ValueTreeRowCollection.cs (8 hunks)
- src/ScriptEngine/Machine/GenericIValueComparer.cs (3 hunks)
- tests/valuetable.os (4 hunks)
- src/OneScript.StandardLibrary/Collections/ValueTable/ColumnNotFoundException.cs
Learnt from: Mr-Rm Repo: EvilBeaver/OneScript PR: 1578 File: src/ScriptEngine/Machine/GenericIValueComparer.cs:55-64 Timestamp: 2025-09-04T11:15:14.305Z Learning: In OneScript, all objects that implement `IValue` are descendants of `BslValue`, making casting from `IValue` to `BslValue` type-safe.
Learnt from: Mr-Rm Repo: EvilBeaver/OneScript PR: 1578 File: src/ScriptEngine/Machine/GenericIValueComparer.cs:55-64 Timestamp: 2025-09-04T11:15:14.305Z Learning: In OneScript, all objects that implement `IValue` are descendants of `BslValue`, making casting from `IValue` to `BslValue` type-safe.
Applied to files:
- src/OneScript.StandardLibrary/Collections/ValueTree/ValueTreeColumnCollection.cs
- src/ScriptEngine/Machine/GenericIValueComparer.cs
- src/OneScript.Core/Values/IBslComparable.cs
- src/OneScript.StandardLibrary/Collections/ValueTree/ValueTreeRowCollection.cs
Learnt from: Mr-Rm Repo: EvilBeaver/OneScript PR: 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.
Applied to files:
- src/OneScript.Core/Values/IBslComparable.cs
Learnt from: Mr-Rm Repo: EvilBeaver/OneScript PR: 1575 File: src/OneScript.Core/Values/BslDateValue.cs:48-48 Timestamp: 2025-08-25T07:41:13.053Z Learning: BslNullValue, BslSkippedParameterValue, and BslUndefinedValue in OneScript use singleton patterns with static Instance properties and ReferenceEquals-based equality, so they should not override GetHashCode().
Applied to files:
- src/OneScript.Core/Values/IBslComparable.cs
Learnt from: Mr-Rm Repo: EvilBeaver/OneScript PR: 1575 File: src/OneScript.Core/Values/BslDateValue.cs:48-48 Timestamp: 2025-08-25T07:41:13.053Z Learning: BslBooleanValue in OneScript uses a flyweight pattern with static True/False instances and reference-based equality, so it should not override GetHashCode().
Applied to files:
- src/OneScript.Core/Values/IBslComparable.cs
Learnt from: Mr-Rm Repo: EvilBeaver/OneScript PR: 1578 File: src/ScriptEngine/Machine/GenericIValueComparer.cs:55-64 Timestamp: 2025-09-04T11:15:14.305Z Learning: In OneScript, `IValueReference` does not inherit from `IValue`, so it cannot be passed to methods expecting `IValue` parameters.
Applied to files:
- src/OneScript.StandardLibrary/Collections/ValueTree/ValueTreeRowCollection.cs
src/OneScript.StandardLibrary/Collections/ValueTable/CollectionIndexes.cs (1)src/OneScript.StandardLibrary/Collections/ValueTree/ValueTreeColumnCollection.cs (1)135-147: Unified column lookup failure handling looks good.
Routing the null-field case through ColumnException.WrongColumnName keeps index creation aligned with the new standardized column exceptions.
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumn.cs (1)42-65: Consistent duplicate-name signalling.
Switching both Add/Insert guards to ColumnException.DuplicatedColumnName keeps ValueTree column management in step with the shared exception helpers.
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumnCollection.cs (1)62-65: Name conflict paths now align with ColumnException.
Using ColumnException.WrongColumnName() here means renames surface the same standardized error seen elsewhere in the table API.
src/OneScript.StandardLibrary/Collections/ValueTree/ValueTreeColumn.cs (1)50-73: ValueTable duplicate-guard is aligned.
Both Add and Insert now emit ColumnException.DuplicatedColumnName, matching the shared column exception surface.
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cs (1)62-64: Rename guard now uses the shared exception.
The duplicate-name check moves to ColumnException.WrongColumnName, keeping ValueTree columns consistent with the rest of the collection stack.
src/OneScript.StandardLibrary/Collections/Exceptions/ColumnException.cs (1)171-171: Column validation refactor looks solid.
Swapping the bespoke runtime exceptions for ColumnException factories, tightening the GroupBy cleanup with RemoveRange, and preferring _rows.Count keep the column API consistent and avoids extra LINQ overhead.
Also applies to: 276-276, 378-379, 396-396, 471-494, 615-616
src/OneScript.StandardLibrary/Collections/ValueTree/ValueTreeRowCollection.cs (1)13-22: LGTM! Clean exception class structure.
The class appropriately derives from RuntimeException and provides standard constructors. The factory method pattern for creating localized exceptions is a good design choice.
434-448: Don't break the script-visible Sort signature
Making the context method take IBslProcess changes the public signature that scripts call (Rows.Sort("Колонка")). Unless there is a matching engine change that auto-injects the process (I don’t see one in this PR), existing script code will now fail with an argument-count mismatch, and the recursive call will pass null, causing _comparer’s presentation path to blow up. Please keep the exported method signature as before and resolve the process internally (e.g., via a private helper), or point me to the runtime binder change that guarantees automatic process injection.
Sorry, something went wrong.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@EvilBeaver @Mr-Rm была ли учтена ошибка при некорректной сортировки английских слов с русскими? До Ожидалось: Крайне критичный баг. |
Sorry, something went wrong.
|
Не понятно, почему это крайне критичный баг? Крайне критичный, это когда ничего не работает, а цикл вместо трех итераций делает восемь. |
Sorry, something went wrong.
|
Проблема в том, что один и тот же код String.Compare(value, other, StringComparison.CurrentCulture);в .Net Framework 4.8 (OneScript v1) и в .Net 6 (OneScript v2) может давать - а при сравнении русских и английских строк и даёт - разный результат. |
Sorry, something went wrong.
@Mr-Rm ИИ совет дает: Может такой вариант затестировать? |
Sorry, something went wrong.
к PR #1584: сортировка строк разных алфавитов
(cherry picked from commit ee2df24)
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
Chores