Исправления и добавления в СписокЗначений#1710
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesValueList behavior
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ValueListImpl
participant TypeDescription
participant AvailableValues
participant ValueListItem
Caller->>ValueListImpl: Insert(value)
ValueListImpl->>TypeDescription: AdjustValue(value)
ValueListImpl->>AvailableValues: FindByValue(adjusted value)
ValueListImpl->>ValueListItem: store normalized value
ValueListImpl-->>Caller: updated list
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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
🧹 Nitpick comments (1)
src/OneScript.StandardLibrary/Collections/ValueList/ValueListImpl.cs (1)
87-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd defensive
nullhandling for C# callers.While the OneScript engine passes
BslUndefinedValuewhen a script sets the property toНеопределено, direct C# callers might clear the property by passingnull. The current switch throws anInvalidArgumentTypeexception fornull.Consider treating
nullidentically toBslUndefinedValue.♻️ Proposed refactor
set { switch (value) { + case null: case BslUndefinedValue: _availableValues = null; break; case ValueListImpl vl: _availableValues = vl; break; default: throw RuntimeException.InvalidArgumentType(); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/OneScript.StandardLibrary/Collections/ValueList/ValueListImpl.cs` around lines 87 - 101, Update the property setter containing the switch on value to handle a C# null input the same way as BslUndefinedValue: clear _availableValues and return without throwing. Preserve the existing ValueListImpl assignment and InvalidArgumentType behavior for all other values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/OneScript.StandardLibrary/Collections/ValueList/ValueListImpl.cs`:
- Around line 157-170: Update CreateNewListItem so
_availableValues.FindByValue(newValue) is treated as a lookup result: use the
matched ValueListItem.Value when found, and retain or reapply
ListValueType.AdjustValue for a missing result such as BslUndefinedValue. Ensure
the returned ValueListItem.Value is never another ValueListItem.
---
Nitpick comments:
In `@src/OneScript.StandardLibrary/Collections/ValueList/ValueListImpl.cs`:
- Around line 87-101: Update the property setter containing the switch on value
to handle a C# null input the same way as BslUndefinedValue: clear
_availableValues and return without throwing. Preserve the existing
ValueListImpl assignment and InvalidArgumentType behavior for all other values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: af84cea8-c1d8-40b8-ade6-4b86459beef3
📒 Files selected for processing (4)
src/OneScript.StandardLibrary/Collections/ValueList/ValueListImpl.cssrc/OneScript.StandardLibrary/Collections/ValueList/ValueListItem.cssrc/OneScript.StandardLibrary/TypeDescriptions/TypeDescription.cstests/value-list.os
There was a problem hiding this comment.
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/ValueList/ValueListImpl.cs (1)
157-173: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDefensively handle
nullfor thepresentationparameter.Direct invocations to
AddorInsertfrom C# code (where the defaultnullis utilized) will result inValueListItem.Presentationbeing set tonull. A subsequent call toSortByPresentation()will throw aNullReferenceExceptionwhen it invokesx.Presentation.CompareTo(...).Normalize it to
string.Emptyto ensure stability. Based on learnings, explicitly handlenullfor optional string parameters to protect against direct C# invocations.🛡️ Proposed fix
private ValueListItem CreateNewListItem(IValue value, string presentation, bool check, IValue picture) { var newValue = ListValueType.AdjustValue(value); if (_availableValues is not null) { var foundItem = _availableValues.FindByValue(newValue); newValue = foundItem is ValueListItem li ? li.Value : ListValueType.AdjustValue(); } return new ValueListItem { Value = newValue, - Presentation = presentation, + Presentation = presentation ?? string.Empty, Check = check, Picture = picture }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/OneScript.StandardLibrary/Collections/ValueList/ValueListImpl.cs` around lines 157 - 173, Update CreateNewListItem so a null presentation argument is normalized to string.Empty before assigning ValueListItem.Presentation. Preserve the supplied presentation text for non-null values and keep the existing value adjustment and item construction behavior unchanged.Source: Learnings
🧹 Nitpick comments (2)
src/OneScript.StandardLibrary/Collections/ValueList/ValueListImpl.cs (2)
396-411: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse expression-bodied methods for the factory methods.
Since C# 12 language features are permitted, simplifying these methods to expression bodies will eliminate boilerplate and improve readability. As per coding guidelines, C# 12 language features can be applied when generating code for projects other than VSCode.DebugAdapter.
✨ Proposed refactor
[ScriptConstructor] - public static ValueListImpl Constructor() - { - return new ValueListImpl(); - } + public static ValueListImpl Constructor() => new(); - public static RuntimeException IndexedIsReadonlyException() - { - return new("Индексированное значение доступно только для чтения", "Indexed value is read-only"); - } + public static RuntimeException IndexedIsReadonlyException() => + new("Индексированное значение доступно только для чтения", "Indexed value is read-only"); - public static RuntimeException ElementDoesntBelongException() - { - return new("Элемент не принадлежит списку значений", "Element does not belong to values list"); - } + public static RuntimeException ElementDoesntBelongException() => + new("Элемент не принадлежит списку значений", "Element does not belong to values list");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/OneScript.StandardLibrary/Collections/ValueList/ValueListImpl.cs` around lines 396 - 411, Convert the ValueListImpl factory-style methods Constructor, IndexedIsReadonlyException, and ElementDoesntBelongException to expression-bodied members, preserving their current return values and exception messages.Source: Coding guidelines
330-345: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAvoid reversing integer comparisons by multiplying by -1.
Multiplying a comparison result by
-1is a known anti-pattern because it can fail if the underlying comparer returnsint.MinValue(-int.MinValue == int.MinValuedue to integer overflow). The idiomatic and safe way to reverse the sort order is to swap the arguments, which is already correctly done inSortByPresentation.♻️ Proposed refactor
private class ItemComparator : IComparer<ValueListItem> { readonly GenericIValueComparer _comparer; - readonly int _direction; + readonly bool _ascending; public ItemComparator(IBslProcess process, bool ascending = true) { _comparer = new GenericIValueComparer(process); - _direction = ascending ? 1 : -1; + _ascending = ascending; } public int Compare(ValueListItem x, ValueListItem y) { - return _comparer.Compare(x.Value, y.Value) * _direction; + return _ascending ? _comparer.Compare(x.Value, y.Value) : _comparer.Compare(y.Value, x.Value); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/OneScript.StandardLibrary/Collections/ValueList/ValueListImpl.cs` around lines 330 - 345, Update ItemComparator.Compare to avoid multiplying the comparer result by _direction; select the argument order passed to GenericIValueComparer.Compare based on the requested ascending or descending order, preserving ascending behavior and safely reversing descending comparisons.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/OneScript.StandardLibrary/Collections/ValueList/ValueListImpl.cs`:
- Around line 157-173: Update CreateNewListItem so a null presentation argument
is normalized to string.Empty before assigning ValueListItem.Presentation.
Preserve the supplied presentation text for non-null values and keep the
existing value adjustment and item construction behavior unchanged.
---
Nitpick comments:
In `@src/OneScript.StandardLibrary/Collections/ValueList/ValueListImpl.cs`:
- Around line 396-411: Convert the ValueListImpl factory-style methods
Constructor, IndexedIsReadonlyException, and ElementDoesntBelongException to
expression-bodied members, preserving their current return values and exception
messages.
- Around line 330-345: Update ItemComparator.Compare to avoid multiplying the
comparer result by _direction; select the argument order passed to
GenericIValueComparer.Compare based on the requested ascending or descending
order, preserving ascending behavior and safely reversing descending
comparisons.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed10b209-ad13-4198-90c1-537f8abb0050
📒 Files selected for processing (2)
src/OneScript.StandardLibrary/Collections/ValueList/ValueListImpl.cstests/value-list.os
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests