Исправления и добавления в СписокЗначений - #1710
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 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 and identifier
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
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
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/ValueList/ValueListImpl.cs (1)
106-108: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate numeric indices before narrowing them.
GetValuenarrowsindex.AsNumber()before checking_items.Count, andIndexByValuecallsdecimal.ToInt32before the same check. Values outside theintrange raiseOverflowExceptioninstead ofRuntimeException.IndexOutOfRange(). Validate the original numeric value as in-range before converting, and reject non-integral indices if only integer list positions are valid.🤖 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 106 - 108, Update ValueListImpl.GetValue and IndexByValue so numeric indexes are validated in their original numeric form before any narrowing conversion; the current numericIndex and decimal.ToInt32 path can throw OverflowException for values outside int range. First confirm the index is an in-range integral list position, then convert and continue with the existing _items.Count bounds check, preserving RuntimeException.IndexOutOfRange() for invalid indices and rejecting non-integral values when only integer positions are supported.
🤖 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 183-184: Update LoadValues to create each item through
CreateNewListItem rather than constructing ValueListItem and assigning Value
directly, so ListValueType and AvailableValues normalization matches Add and
Insert. Preserve the existing source ordering and ID assignment behavior.
---
Outside diff comments:
In `@src/OneScript.StandardLibrary/Collections/ValueList/ValueListImpl.cs`:
- Around line 106-108: Update ValueListImpl.GetValue and IndexByValue so numeric
indexes are validated in their original numeric form before any narrowing
conversion; the current numericIndex and decimal.ToInt32 path can throw
OverflowException for values outside int range. First confirm the index is an
in-range integral list position, then convert and continue with the existing
_items.Count bounds check, preserving RuntimeException.IndexOutOfRange() for
invalid indices and rejecting non-integral values when only integer positions
are supported.
🪄 Autofix
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 Plus
Run ID: d821aa95-80b6-4286-bb26-d6fdea04d7b3
📒 Files selected for processing (3)
src/OneScript.StandardLibrary/Collections/ValueList/ValueListImpl.cssrc/OneScript.StandardLibrary/Collections/ValueList/ValueListItem.cstests/value-list.os
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests