V2: оптимизация ТаблицыЗначений и Рефлектора #1635
Conversation
WalkthroughRefactors ValueTable indexing and column management: adds per-column index tracking and APIs, changes column constructor signature, centralizes column-name validation, adjusts index lifecycle/population and index-notification gating, and replaces several internal collections with concrete or sorted types. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas needing extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
Я думаю, что нет, но наверное надо спросить тех, кто им активно пользуется: @sfaqer @nixel2007 |
| var annotationsTable = new ValueTable(); | ||
| annotationsTable.Columns.Add("Имя"); | ||
| annotationsTable.Columns.Add("Параметры"); | ||
| annotationsTable.Columns.AddUnchecked("Имя","Имя"); |
There was a problem hiding this comment.
Еще можно, наверное, оптимизировать СкопироватьКолонки, чтобы там был минимум проверок (они сделаны для исходной таблицы) потом таблицу с нужной структурой закешировать в рефлекторе, а инстансы создавать через СкопироватьКолонки.
There was a problem hiding this comment.
Да, рассматривал подобный вариант. Тоже надо отдельную функцию заводить.
Я думаю что особого смысла нет. |
|
Тогда уберу. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/OneScript.StandardLibrary/Collections/ValueTable/CollectionIndexes.cs (1)
132-147: Inconsistent field name in exception message.The exception on line 142 uses the untrimmed
fieldNamefrom the split result, but the lookup uses the trimmedname. This could cause confusing error messages with leading/trailing whitespace in the column name.🔎 Proposed fix to use trimmed name in exception
foreach (var fieldName in fieldNames) { var name = fieldName.Trim(); - var field = source.GetField(name) ?? throw ColumnException.WrongColumnName(fieldName); + var field = source.GetField(name) ?? throw ColumnException.WrongColumnName(name); fields.Add(field); }
🧹 Nitpick comments (6)
src/OneScript.StandardLibrary/TypeDescriptions/TypeDescriptionBuilder.cs (1)
40-48: Consider null-safety for thetypesparameter.The
AddTypesmethod iterates overtypeswithout a null check. WhileSourceDescriptioncallssource.TypesInternal()which returns a non-nullIEnumerable, the method is public within the class and could be called with null from other paths.🔎 Proposed defensive null check
public TypeDescriptionBuilder AddTypes(IEnumerable<BslTypeValue> types) { + if (types == null) + return this; + foreach (var type in types) { if (type.TypeValue.ImplementingClass != typeof(BslUndefinedValue)) _types.Add(type); } return this; }src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableRow.cs (1)
90-100: Good optimization, but minor code duplication.The optimization to skip index operations for non-indexable columns is the key performance improvement. The logic is correct.
Consider extracting the common assignment to reduce duplication:
🔎 Proposed refactor to reduce duplication
public void Set(ValueTableColumn column, IValue value) { if (column.IsIndexable) { _owner.Indexes.ElementRemoved(this); - _data[column] = column.ValueType.AdjustValue(value); - _owner.Indexes.ElementAdded(this); } - else - _data[column] = column.ValueType.AdjustValue(value); + + _data[column] = column.ValueType.AdjustValue(value); + + if (column.IsIndexable) + { + _owner.Indexes.ElementAdded(this); + } }src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs (1)
43-46: Minor inefficiency:ToHashSet()allocates on every call.
CanBeUsedForis called duringFindRowsto locate suitable indexes. Creating a newHashSeton each call adds allocation overhead.🔎 Proposed optimization using HashSet for _fields
Consider caching the fields as a
HashSet<IValue>alongside the list, or use a more direct comparison if the number of fields is typically small:-return _fields.Count>0 && _fields.ToHashSet().IsSubsetOf(searchFields); +return _fields.Count > 0 && _fields.All(f => searchFields.Contains(f));Or if
searchFieldsis typically a small collection and you want the subset semantics:+private readonly HashSet<IValue> _fieldsSet = new HashSet<IValue>(); // populate _fieldsSet alongside _fields in constructorsrc/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cs (1)
69-76: Good optimization, butInsert()is inconsistent.
Add()now checks if there are any indexes before callingElementAdded, butInsert()at line 93 unconditionally callsIndexes.ElementAdded(row). Consider applying the same optimization for consistency.🔎 Proposed fix for Insert()
public ValueTableRow Insert(int index) { var row = new ValueTableRow(this); if (index < 0 || index > _rows.Count) _rows.Add(row); // для совместимости с 1С, хотя логичней было бы исключение else _rows.Insert(index, row); - Indexes.ElementAdded(row); + if (Indexes.Count() > 0) + Indexes.ElementAdded(row); return row; }src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumn.cs (1)
27-27: Consider restrictingIsIndexablesetter access.The
IsIndexableproperty has a public setter, allowing external code to modify indexing behavior at any time. If this is intentional to support dynamic index control, it's fine. However, if indexability should only be set during column creation, consider making the setterinternalor replacing the auto-property with{ get; internal set; }to prevent unintended modifications from user scripts.📝 Alternative with restricted access
-public bool IsIndexable { get; set; } +public bool IsIndexable { get; internal set; }src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumnCollection.cs (1)
38-50: Consider adding XML documentation toAddUncheckedmethods.These public methods bypass validation for performance optimization, which is appropriate for internal scenarios like the Reflector. However, they lack XML documentation comments explaining:
- When they should be used vs. the regular
Addmethod- That they skip name validation
- The implications of bypassing validation
Adding documentation would help prevent misuse and clarify intent.
📝 Suggested documentation
+/// <summary> +/// Adds a column without name validation. Use only when column name validity is guaranteed. +/// </summary> +/// <param name="name">Column name (not validated)</param> +/// <param name="title">Column title</param> +/// <param name="type">Type description</param> +/// <returns>The created column</returns> public ValueTableColumn AddUnchecked(string name, string title, TypeDescription type = null)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs(2 hunks)src/OneScript.StandardLibrary/Collections/ValueTable/CollectionIndexes.cs(2 hunks)src/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cs(6 hunks)src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumn.cs(1 hunks)src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumnCollection.cs(4 hunks)src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableRow.cs(1 hunks)src/OneScript.StandardLibrary/Reflector.cs(18 hunks)src/OneScript.StandardLibrary/TypeDescriptions/TypeComparer.cs(1 hunks)src/OneScript.StandardLibrary/TypeDescriptions/TypeDescription.cs(1 hunks)src/OneScript.StandardLibrary/TypeDescriptions/TypeDescriptionBuilder.cs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (.cursor/rules/langversion.mdc)
Do not use C# language features that are not available in C# 8 when generating code for projects using .NET 8.0 with LangVersion 8
Files:
src/OneScript.StandardLibrary/TypeDescriptions/TypeComparer.cssrc/OneScript.StandardLibrary/Collections/ValueTable/ValueTableRow.cssrc/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumn.cssrc/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumnCollection.cssrc/OneScript.StandardLibrary/TypeDescriptions/TypeDescription.cssrc/OneScript.StandardLibrary/Collections/ValueTable/CollectionIndexes.cssrc/OneScript.StandardLibrary/Reflector.cssrc/OneScript.StandardLibrary/TypeDescriptions/TypeDescriptionBuilder.cssrc/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cssrc/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs
🧠 Learnings (2)
📚 Learning: 2025-12-18T16:13:05.448Z
Learnt from: Mr-Rm
Repo: EvilBeaver/OneScript PR: 1636
File: src/OneScript.StandardLibrary/StringOperations.cs:155-157
Timestamp: 2025-12-18T16:13:05.448Z
Learning: Guideline: In OneScript, when a method parameter has a concrete type (e.g., string) and is called from scripts with omitted arguments, the runtime passes an empty string "" rather than null. Direct (non-script) calls to the method may still pass null, so implement defensive null checks for these parameters in public methods that can be called from scripts. Treat empty string as a legitimate value for script calls, and explicitly handle null for direct calls (e.g., fail fast, throw ArgumentNullException, or normalize to "" where appropriate). This should apply to all C# methods that may be invoked from OneScript with optional string parameters across the codebase.
Applied to files:
src/OneScript.StandardLibrary/TypeDescriptions/TypeComparer.cssrc/OneScript.StandardLibrary/Collections/ValueTable/ValueTableRow.cssrc/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumn.cssrc/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumnCollection.cssrc/OneScript.StandardLibrary/TypeDescriptions/TypeDescription.cssrc/OneScript.StandardLibrary/Collections/ValueTable/CollectionIndexes.cssrc/OneScript.StandardLibrary/Reflector.cssrc/OneScript.StandardLibrary/TypeDescriptions/TypeDescriptionBuilder.cssrc/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cssrc/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs
📚 Learning: 2025-12-05T07:24:02.362Z
Learnt from: EvilBeaver
Repo: EvilBeaver/OneScript PR: 1623
File: src/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cs:84-94
Timestamp: 2025-12-05T07:24:02.362Z
Learning: In OneScript's ValueTable.Insert method (src/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cs), when the index parameter is negative or greater than the row count (index < 0 || index > _rows.Count), the new row should be appended to the end of the table. This behavior matches 1C platform compatibility, where out-of-bounds insertion adds rows to the end rather than throwing an exception.
Applied to files:
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableRow.cssrc/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumn.cssrc/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumnCollection.cssrc/OneScript.StandardLibrary/Collections/ValueTable/CollectionIndexes.cssrc/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cs
🧬 Code graph analysis (8)
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableRow.cs (2)
src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs (2)
ElementRemoved(86-93)ElementAdded(73-84)src/OneScript.StandardLibrary/Collections/ValueTable/CollectionIndexes.cs (2)
ElementRemoved(119-125)ElementAdded(111-117)
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumn.cs (3)
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumnCollection.cs (6)
ValueTableColumn(38-43)ValueTableColumn(45-50)ValueTableColumn(167-170)ValueTableColumn(172-177)ValueTableColumn(221-240)ValueTableColumnCollection(33-36)src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableRow.cs (1)
ValueTableColumn(149-152)src/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cs (1)
ValueTableColumn(318-320)
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumnCollection.cs (3)
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumn.cs (1)
ValueTableColumn(29-38)src/OneScript.Core/Commons/Utils.cs (1)
Utils(12-54)src/OneScript.StandardLibrary/Collections/Exceptions/ColumnException.cs (7)
ColumnException(13-53)ColumnException(15-18)ColumnException(20-22)ColumnException(24-29)ColumnException(31-36)ColumnException(38-43)ColumnException(46-51)
src/OneScript.StandardLibrary/TypeDescriptions/TypeDescription.cs (6)
src/OneScript.StandardLibrary/TypeDescriptions/TypeDescriptionBuilder.cs (1)
TypeDescription(83-102)src/OneScript.Core/Values/BslTypeValue.cs (3)
BslTypeValue(12-54)BslTypeValue(16-19)BslTypeValue(21-21)src/OneScript.StandardLibrary/TypeDescriptions/NumberQualifiers.cs (2)
NumberQualifiers(21-28)ScriptConstructor(92-104)src/OneScript.StandardLibrary/TypeDescriptions/StringQualifiers.cs (2)
StringQualifiers(19-24)ScriptConstructor(84-89)src/OneScript.StandardLibrary/TypeDescriptions/DateQualifiers.cs (2)
DateQualifiers(20-23)ScriptConstructor(70-74)src/OneScript.StandardLibrary/TypeDescriptions/BinaryDataQualifiers.cs (2)
BinaryDataQualifiers(18-23)ScriptConstructor(50-54)
src/OneScript.StandardLibrary/Collections/ValueTable/CollectionIndexes.cs (3)
src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs (2)
Count(120-123)IValue(106-118)src/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cs (4)
IValue(264-279)IValue(281-293)IValue(745-748)IValue(763-766)src/OneScript.StandardLibrary/Collections/Exceptions/ColumnException.cs (7)
ColumnException(13-53)ColumnException(15-18)ColumnException(20-22)ColumnException(24-29)ColumnException(31-36)ColumnException(38-43)ColumnException(46-51)
src/OneScript.StandardLibrary/TypeDescriptions/TypeDescriptionBuilder.cs (2)
src/OneScript.StandardLibrary/TypeDescriptions/TypeComparer.cs (2)
TypeComparer(14-64)TypeComparer(54-62)src/OneScript.StandardLibrary/TypeDescriptions/TypeDescription.cs (1)
IEnumerable(79-82)
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cs (3)
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumnCollection.cs (6)
Count(122-122)ValueTableColumn(38-43)ValueTableColumn(45-50)ValueTableColumn(167-170)ValueTableColumn(172-177)ValueTableColumn(221-240)src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableRow.cs (4)
Count(27-30)Count(32-32)ValueTableColumn(149-152)ValueTableRow(22-25)src/OneScript.StandardLibrary/Collections/Exceptions/ColumnException.cs (7)
ColumnException(13-53)ColumnException(15-18)ColumnException(20-22)ColumnException(24-29)ColumnException(31-36)ColumnException(38-43)ColumnException(46-51)
src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs (1)
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumn.cs (1)
ValueTableColumn(29-38)
🔇 Additional comments (20)
src/OneScript.StandardLibrary/TypeDescriptions/TypeComparer.cs (1)
17-17: LGTM!Using the concrete
Dictionary<,>type instead of the interface for a private static field is a valid micro-optimization that avoids interface dispatch overhead.src/OneScript.StandardLibrary/TypeDescriptions/TypeDescription.cs (2)
157-182: LGTM!Direct construction of
TypeDescriptionfor single-type descriptors (StringType,IntegerType,BooleanType) is a valid optimization that avoids the builder overhead while maintaining equivalent behavior. All qualifiers are properly initialized.
183-184: Attribute placement looks correct.The
[ScriptConstructor]attribute is correctly placed immediately before theConstructormethod declaration.src/OneScript.StandardLibrary/TypeDescriptions/TypeDescriptionBuilder.cs (2)
24-26: LGTM!Using a
SortedSet<BslTypeValue>with a staticTypeComparerinstance is a clean optimization that:
- Guarantees uniqueness automatically (no explicit deduplication needed)
- Maintains consistent ordering
- Reuses a single comparer instance across all builder instances
50-57: LGTM!The simplified
RemoveTypesimplementation using a foreach loop is cleaner and works correctly withSortedSet.Remove().src/OneScript.StandardLibrary/Collections/ValueTable/CollectionIndexes.cs (1)
42-42: LGTM!Using the
Countproperty directly instead of calling a method is a minor but valid improvement.src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs (1)
95-95: LGTM!Simplifying
Clear()to an expression-bodied member is a clean style improvement.src/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cs (2)
318-321: LGTM!The
GetColumnOrThrowhelper centralizes column lookup and exception throwing, reducing code duplication acrossCheckFilterCriteria,ColumnsMap, andGetSortRules. The expression-bodied syntax is clean and appropriate.
327-327: LGTM!All usages of
GetColumnOrThroware appropriate replacements for the previous inline lookup-and-throw patterns.Also applies to: 367-367, 656-656
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumn.cs (2)
24-24: LGTM: Making_valueTypeimmutable.The readonly modifier ensures the value type cannot be changed after construction, which is correct for this scenario.
29-38: LGTM: Simplified constructor signature.Removing the
idparameter simplifies the API and aligns with the updated column creation logic inValueTableColumnCollection. The defaultIsIndexable = falsemakes sense as an opt-in optimization.src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumnCollection.cs (3)
28-28: LGTM: Static comparer for case-insensitive column names.Using a static
StringComparer.OrdinalIgnoreCaseis efficient and appropriate for column name lookups. This avoids creating comparer instances per collection.
53-60: LGTM: Centralized validation logic.The
CheckColumnNamehelper effectively consolidates validation logic fromAddandInsertmethods, following the DRY principle and making validation consistent.
132-132: LGTM: Consistent IValue return type.Returning
ValueFactory.Create()(undefined) instead ofnullwhen a column is not found is more consistent with OneScript's type system and matches the documented behavior.src/OneScript.StandardLibrary/Reflector.cs (6)
31-31: Minor: Formatting improvement in attribute.The space after the comma in
[ContextClass("Рефлектор", "Reflector")]improves readability.
145-154: LGTM: Index constants and unchecked column additions.The introduction of
annotNameColumnIndexandannotParamsColumnIndexconstants combined withAddUncheckedis an excellent optimization:
- Index-based access avoids repeated string comparisons
- AddUnchecked skips validation overhead for known-safe internal table construction
- Constants make the code more maintainable
The indices correctly match the column creation order (Имя=0, Параметры=1).
159-160: LGTM: Consistent use of index-based column access.Using
FindColumnByIndexwith the predefined constants is more efficient than name-based lookups and ensures consistency with the index constants defined earlier.
191-196: LGTM: Optimized annotation value construction.Caching
expandedValue.ColumnsinexpandedValueColumnsand using index-based access reduces redundant property accesses and lookups, improving performance.
351-356: LGTM: Consistent use ofAddUncheckedfor internal table construction.Using
AddUncheckedthroughout the methods table construction is appropriate because:
- The Reflector creates read-only metadata tables
- Column names are hardcoded and guaranteed to be valid
- This optimization aligns with the PR's goal of improving performance when indices aren't needed
According to the PR discussion, avoiding type descriptions (
ОписанияТипов) in these tables further improves performance sinceAdjustValueis expensive and unnecessary for read-only metadata.Based on learnings: The PR objectives mention that type descriptions will be removed from Reflector tables since they're not written to, and
AdjustValueis time-consuming.
86-87: Minor: Formatting consistency.The added line break after variable initialization improves code readability by visually separating variable declarations from subsequent logic.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs (2)
21-25: Minor style inconsistency in access modifiers.Line 21 omits the
privatekeyword while line 24 explicitly includes it. Both are functionally equivalent sinceprivateis the default, but consider being consistent.🔎 Proposed fix for consistency
- readonly List<IValue> _fields = new List<IValue>(); + private readonly List<IValue> _fields = new List<IValue>();
43-46: Minor formatting: add spacing around operator.🔎 Proposed fix
- return _fields.Count>0 && _fields.ToHashSet().IsSubsetOf(searchFields); + return _fields.Count > 0 && _fields.ToHashSet().IsSubsetOf(searchFields);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (.cursor/rules/langversion.mdc)
Do not use C# language features that are not available in C# 8 when generating code for projects using .NET 8.0 with LangVersion 8
Files:
src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs
🧠 Learnings (1)
📚 Learning: 2025-12-18T16:13:05.448Z
Learnt from: Mr-Rm
Repo: EvilBeaver/OneScript PR: 1636
File: src/OneScript.StandardLibrary/StringOperations.cs:155-157
Timestamp: 2025-12-18T16:13:05.448Z
Learning: Guideline: In OneScript, when a method parameter has a concrete type (e.g., string) and is called from scripts with omitted arguments, the runtime passes an empty string "" rather than null. Direct (non-script) calls to the method may still pass null, so implement defensive null checks for these parameters in public methods that can be called from scripts. Treat empty string as a legitimate value for script calls, and explicitly handle null for direct calls (e.g., fail fast, throw ArgumentNullException, or normalize to "" where appropriate). This should apply to all C# methods that may be invoked from OneScript with optional string parameters across the codebase.
Applied to files:
src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs
🧬 Code graph analysis (1)
src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs (2)
src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndexKey.cs (3)
CollectionIndexKey(15-82)CollectionIndexKey(20-24)CollectionIndexKey(37-48)src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumn.cs (1)
ValueTableColumn(29-38)
🔇 Additional comments (2)
src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs (2)
27-41: Initialization order issue has been fixed.The constructor now correctly populates
_fields(lines 29-34) before iterating over_sourceand callingElementAdded(lines 37-40). This ensuresCollectionIndexKey.Extractreceives a populated_fieldscollection when building index keys.
95-95: LGTM!Concise expression-bodied member syntax is appropriate for this single-statement method.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cs (1)
181-183: Suggest applyingGetColumnOrThrowconsistently inGetProcessingColumnList.Lines 328, 368, and 657 now use the new
GetColumnOrThrowhelper for consistent column validation. However, lines 181-183 inGetProcessingColumnListstill use the old pattern:var Column = Columns.FindColumnByName(name); if (Column == null) throw ColumnException.WrongColumnName(column_name);Consider refactoring this to use
GetColumnOrThrow(name)for consistency, noting that the error message currently referencescolumn_name(untrimmed) rather thanname(trimmed), which may need to be preserved if intentional.🔎 Proposed refactor for consistency
-var Column = Columns.FindColumnByName(name); -if (Column == null) - throw ColumnException.WrongColumnName(column_name); +var Column = GetColumnOrThrow(name);If preserving the untrimmed
column_namein the error message is important, consider adding an overload toGetColumnOrThrowthat accepts a custom error message parameter.Also applies to: 328-328, 351-351, 368-368, 657-657
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs(2 hunks)src/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cs(8 hunks)src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumn.cs(1 hunks)src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableRow.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs
- src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableRow.cs
- src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumn.cs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (.cursor/rules/langversion.mdc)
Do not use C# language features that are not available in C# 8 when generating code for projects using .NET 8.0 with LangVersion 8
Files:
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cs
🧠 Learnings (2)
📚 Learning: 2025-12-05T07:24:02.362Z
Learnt from: EvilBeaver
Repo: EvilBeaver/OneScript PR: 1623
File: src/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cs:84-94
Timestamp: 2025-12-05T07:24:02.362Z
Learning: In OneScript's ValueTable.Insert method (src/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cs), when the index parameter is negative or greater than the row count (index < 0 || index > _rows.Count), the new row should be appended to the end of the table. This behavior matches 1C platform compatibility, where out-of-bounds insertion adds rows to the end rather than throwing an exception.
Applied to files:
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cs
📚 Learning: 2025-12-18T16:13:05.448Z
Learnt from: Mr-Rm
Repo: EvilBeaver/OneScript PR: 1636
File: src/OneScript.StandardLibrary/StringOperations.cs:155-157
Timestamp: 2025-12-18T16:13:05.448Z
Learning: Guideline: In OneScript, when a method parameter has a concrete type (e.g., string) and is called from scripts with omitted arguments, the runtime passes an empty string "" rather than null. Direct (non-script) calls to the method may still pass null, so implement defensive null checks for these parameters in public methods that can be called from scripts. Treat empty string as a legitimate value for script calls, and explicitly handle null for direct calls (e.g., fail fast, throw ArgumentNullException, or normalize to "" where appropriate). This should apply to all C# methods that may be invoked from OneScript with optional string parameters across the codebase.
Applied to files:
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cs
🔇 Additional comments (3)
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cs (3)
87-95: LGTM: Insert optimization improves efficiency.The refactored logic now creates the
ValueTableRowonly when the index is valid, avoiding unnecessary object allocation when falling back toAdd(). The out-of-bounds behavior correctly delegates toAdd()for 1C platform compatibility, and the index notification guard is consistent with theAdd()method.Based on learnings, this insertion behavior (out-of-bounds → append) matches the required 1C compatibility.
319-322: LGTM: Centralized column lookup improves consistency.The new
GetColumnOrThrowhelper consolidates column lookup and exception handling, reducing code duplication and ensuring consistent error messages across multiple call sites.
73-74: Verify thatIndexes.Count()is O(1) and confirm the optimization is sound.The guard
if (Indexes.Count() > 0)at lines 73-74 and 93-94 is a good optimization.CollectionIndexes.Count()returns_indexes.Countdirectly, where_indexesis aList<CollectionIndex>, making it an O(1) operation. This avoids notifying indexes when none exist, effectively implementing the PR's goal of optimizing for cases without indexes.Also applies to: 93-94
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs (2)
45-45: LGTM: CanBeUsedFor logic refactored.The refactored logic
_fields.All(f => searchFields.Contains(f))is functionally equivalent to checking if_fieldsis a subset ofsearchFields. This is a clean refactoring with no behavioral change.
110-110: LGTM: Expression-bodied member syntax.Converting
Clear()to an expression-bodied form is a clean refactoring that improves readability without changing behavior.src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumn.cs (1)
24-24: LGTM: _valueType made readonly.Making
_valueTypereadonly is a good practice since it's only assigned in the constructor. This prevents accidental modifications and clarifies the immutability of this field.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs(3 hunks)src/OneScript.StandardLibrary/Collections/ValueTable/CollectionIndexes.cs(2 hunks)src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumn.cs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (.cursor/rules/langversion.mdc)
Do not use C# language features that are not available in C# 8 when generating code for projects using .NET 8.0 with LangVersion 8
Files:
src/OneScript.StandardLibrary/Collections/ValueTable/CollectionIndexes.cssrc/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cssrc/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumn.cs
🧠 Learnings (2)
📚 Learning: 2025-12-05T07:24:02.362Z
Learnt from: EvilBeaver
Repo: EvilBeaver/OneScript PR: 1623
File: src/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cs:84-94
Timestamp: 2025-12-05T07:24:02.362Z
Learning: In OneScript's ValueTable.Insert method (src/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cs), when the index parameter is negative or greater than the row count (index < 0 || index > _rows.Count), the new row should be appended to the end of the table. This behavior matches 1C platform compatibility, where out-of-bounds insertion adds rows to the end rather than throwing an exception.
Applied to files:
src/OneScript.StandardLibrary/Collections/ValueTable/CollectionIndexes.cssrc/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumn.cs
📚 Learning: 2025-12-18T16:13:05.448Z
Learnt from: Mr-Rm
Repo: EvilBeaver/OneScript PR: 1636
File: src/OneScript.StandardLibrary/StringOperations.cs:155-157
Timestamp: 2025-12-18T16:13:05.448Z
Learning: Guideline: In OneScript, when a method parameter has a concrete type (e.g., string) and is called from scripts with omitted arguments, the runtime passes an empty string "" rather than null. Direct (non-script) calls to the method may still pass null, so implement defensive null checks for these parameters in public methods that can be called from scripts. Treat empty string as a legitimate value for script calls, and explicitly handle null for direct calls (e.g., fail fast, throw ArgumentNullException, or normalize to "" where appropriate). This should apply to all C# methods that may be invoked from OneScript with optional string parameters across the codebase.
Applied to files:
src/OneScript.StandardLibrary/Collections/ValueTable/CollectionIndexes.cssrc/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cssrc/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumn.cs
🧬 Code graph analysis (3)
src/OneScript.StandardLibrary/Collections/ValueTable/CollectionIndexes.cs (1)
src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs (2)
IValue(121-133)ExcludeFields(79-86)
src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs (2)
src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndexKey.cs (3)
CollectionIndexKey(15-82)CollectionIndexKey(20-24)CollectionIndexKey(37-48)src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumn.cs (3)
ValueTableColumn(32-40)AddToIndex(29-29)DeleteFromIndex(30-30)
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumn.cs (2)
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumnCollection.cs (6)
ValueTableColumn(38-43)ValueTableColumn(45-50)ValueTableColumn(167-170)ValueTableColumn(172-177)ValueTableColumn(221-240)ValueTableColumnCollection(33-36)src/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cs (1)
ValueTableColumn(319-321)
🔇 Additional comments (9)
src/OneScript.StandardLibrary/Collections/ValueTable/CollectionIndexes.cs (4)
39-43: LGTM!Good micro-optimization: direct property access
_indexes.Countavoids the overhead of the LINQCount()extension method call.
45-51: LGTM!Correct ordering: fields are excluded from the index before removal, ensuring proper cleanup and consistency with the new index lifecycle management.
53-60: LGTM!Consistent with the
Deletemethod: ensures all indexes have their fields properly excluded before the collection is cleared.
137-152: Subtle behavioral change for whitespace-only field names.The previous implementation used
string.IsNullOrWhiteSpace(fieldName)inside the loop to silently skip whitespace-only entries (e.g.,"col1, ,col2"would process onlycol1andcol2).The new implementation uses
RemoveEmptyEntries, which only removes truly empty strings—whitespace-only entries like" "still pass through, get trimmed to"", and will now throwWrongColumnNamewhenGetField("")returns null.This is arguably more correct (errors on malformed input rather than silently ignoring), but it's a breaking change. If backward compatibility is required, restore the whitespace filtering:
Option to restore previous behavior
- var fieldNames = fieldList.Split(',', System.StringSplitOptions.RemoveEmptyEntries); + var fieldNames = fieldList.Split(','); foreach (var fieldName in fieldNames) { - var name = fieldName.Trim(); - var field = source.GetField(name) ?? throw ColumnException.WrongColumnName(fieldName); - fields.Add(field); + var name = fieldName.Trim(); + if (string.IsNullOrEmpty(name)) + continue; + var field = source.GetField(name) ?? throw ColumnException.WrongColumnName(fieldName); + fields.Add(field); }src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs (3)
24-25: LGTM: Type change to concrete Dictionary.The change from
IDictionarytoDictionaryfor the internal_datafield is appropriate. Since this field is not exposed publicly, using the concrete type reduces unnecessary abstraction and may provide minor performance benefits.
27-41: LGTM: Constructor initialization order is now correct.The initialization order issue flagged in previous reviews has been properly addressed. Fields are now populated into
_fields(lines 29-34) before iterating_sourceand callingElementAdded()(lines 37-40), ensuring thatCollectionIndexKey.Extract()will produce valid keys from the start.
68-74: The while loop correctly implements reference counting. When the same column is added multiple times to_fields(which the constructor allows),AddToIndex()is called once per occurrence, incrementingValueTableColumn._indicesCountaccordingly. The while loop removes all occurrences and callsDeleteFromIndex()the same number of times, properly decrementing the counter back to zero. This is intentional and correct behavior—no action needed.src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumn.cs (2)
27-30: LGTM: Clean index tracking implementation.The index tracking implementation is well-designed:
_indicesCounttracks how many indexes reference this columnIsIndexableprovides a fast O(1) check for index participationDeleteFromIndex()includes defensive logic to prevent negative countsThis design supports the PR's optimization goal by enabling fast checks for columns not participating in any index.
32-32: LGTM: Constructor signature simplified.Removing the
idparameter simplifies the constructor and is consistent with the updated callers inValueTableColumnCollection.cs(lines 39 and 46), where columns are now created without auto-incremented IDs.
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
src/OneScript.StandardLibrary/Collections/ValueTable/CollectionIndexes.cs
Show resolved
Hide resolved
3 participants
Нужны ли ОписанияТипов в таблицах, формируемых Рефлектором? Ведь запись в них не производится.
Summary by CodeRabbit
Bug Fixes
Improvements
Public API
✏️ Tip: You can customize this high-level summary in your review settings.
Note
Speeds up ValueTable by avoiding unnecessary index updates via index-aware columns, simplifies Reflector table creation, and refactors TypeDescription building with a SortedSet-based comparer.
ValueTableColumnindex-aware (IsIndexable,AddToIndex,DeleteFromIndex) and updateCollectionIndexto manage column index refs and rebuild on init.Add,Insert,LoadColumn,FillValues).CollectionIndex.ExcludeFields; ensure proper cleanup on index/collection delete/clear.FindRowsto use a suitable index (suitableIndex?.GetData(...)); addGetColumnOrThrowfor consistent column lookup.ValueTable.Columns.AddUnchecked(...)for fast column setup; build annotation tables via column indices; unify property/method table schemas without explicit type descriptions.SortedSet+TypeComparer; filter outUndefined.TypeDescription.StringType/IntegerType/BooleanTypeconstructors; minor comparer/dictionary tweaks.Written by Cursor Bugbot for commit 186a8bf. This will update automatically on new commits. Configure here.