V2: совместимость и оптимизация операций со строками #1636
Conversation
WalkthroughIntroduces a reusable private regex for template processing, adds a public StringOpException class with factory methods, changes StrFind's default occurance from 0 to 1 and adds input validation, and adds tests covering StrFind edge cases. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
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 |
![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/StringOperations.cs (1)
25-26: Consider usingRegexOptions.Compiledfor performance.This static regex is reused across all
StrTemplatecalls. For frequently-used patterns,RegexOptions.Compiledcan improve performance at the cost of slightly increased startup time.🔎 Optional performance improvement:
private static readonly System.Text.RegularExpressions.Regex _templateRe - = new System.Text.RegularExpressions.Regex(@"(%%)|%(\d+)|%\((\d+)\)|%"); + = new System.Text.RegularExpressions.Regex(@"(%%)|%(\d+)|%\((\d+)\)|%", + System.Text.RegularExpressions.RegexOptions.Compiled);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/OneScript.StandardLibrary/StringOperations.cs(14 hunks)tests/stringoperations.os(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/StringOperations.cs
🧬 Code graph analysis (1)
src/OneScript.StandardLibrary/StringOperations.cs (1)
src/ScriptEngine/Machine/ExceptionInfoFactory.cs (1)
Exception(54-63)
🔇 Additional comments (14)
tests/stringoperations.os (7)
51-57: LGTM - comprehensive edge case test coverage added.The new test registrations cover important edge cases for
СтрНайти: empty strings, missing parameters, invalid positions, and occurrence validation. This aligns well with the validation logic added to the implementation.
469-476: LGTM - empty haystack test.The test correctly verifies that searching in an empty string returns 0, which is the expected behavior.
478-485: LGTM - empty needle test.The test correctly verifies that an empty needle returns 1 (first position), matching the implementation in
StrFindwhereneedle.Length == 0returns 1.
487-489: Verify behavior when both haystack and needle are undefined.This test calls
СтрНайти(,)with both parameters missing/undefined and expects result 1. This relies on the implementation treatingnullas empty string for needle (returns 1) and haystack becoming empty string. Confirm this matches intended 1C:Enterprise compatibility.
491-505: LGTM - validates out-of-bounds start position throws exception.The test correctly verifies that passing
startPos = 999on a 13-character string throws an exception, aligned with thestartPos > lencheck in the implementation.
507-521: LGTM - validates zero occurrence throws exception.The test correctly verifies that
occurance = 0throws an exception, matching theRuntimeException.InvalidNthArgumentValue(5)in the implementation.
523-530: Verify negative occurrence behavior is intentional.The test expects
СтрНайти(..., -2)to return 0 (not throw). The implementation handlesoccurance < 0by returning 0, whileoccurance == 0throws. Please verify this asymmetry is consistent with 1C:Enterprise 8.3.27.1936 behavior as mentioned in the PR description.src/OneScript.StandardLibrary/StringOperations.cs (7)
59-59: LGTM - centralized exception handling.Using
StringOpException.StrStartsWith()provides consistent bilingual error messages.
104-109: Null-conditional operator?.may cause issues with null delimiter.When
stringDelimiterisnull,stringDelimiter?.ToCharArray()returnsnull, which is passed toString.Split. In .NET,Split(null, ...)splits on whitespace. If the intent is to return the original string when delimiter is null/empty, this behavior may be unexpected.Verify that splitting on whitespace when delimiter is null matches 1C:Enterprise behavior for
СтрРазделить.
150-153: Empty needle returning 1 - verify 1C compatibility.When
needleis empty, the function returns 1 before checking ifhaystackis empty. This meansStrFind("", "")returns 1, andStrFind(null, "")also returns 1 (if null is coerced to empty string). The testТестДолжен_ВызватьМетод_СтрНайти_СПропущеннымиСтрокамиexpects this behavior.
165-171: Validation logic for startPos and occurance is correct.The bounds checking for
startPos(must be 1 to len) andoccurance(0 throws, negative returns 0) aligns with the test expectations. However, the asymmetric handling (negative returns 0, zero throws) is unusual.
206-206: LGTM - simplified return statement.The ternary expression consolidates the return logic cleanly.
265-273: LGTM - SearchDirection enum unchanged.The formatting change (extra blank line) does not affect functionality.
276-311: StrStartsWith() and StrEndsWith() have swapped language parameters; TemplateSyntax() and TemplateSubst() are correct.The
BilingualStringconstructor expects parameters in order:Russianfirst, thenEnglish.StrStartsWith and StrEndsWith incorrectly pass English first (lines 288-289, 294-295), while TemplateSyntax and TemplateSubst correctly pass Russian first (lines 300-302, 307-309). Fix StrStartsWith and StrEndsWith:
public static StringOpException StrStartsWith() { return new StringOpException(new BilingualString( - "Error calling context method (StrStartsWith): Invalid parameter number '2' value", - "Ошибка при вызове метода контекста (СтрНачинаетсяС): Недопустимое значение параметра номер '2'")); + "Ошибка при вызове метода контекста (СтрНачинаетсяС): Недопустимое значение параметра номер '2'", + "Error calling context method (StrStartsWith): Invalid parameter number '2' value")); } public static StringOpException StrEndsWith() { return new StringOpException(new BilingualString( - "Error calling context method (StrEndsWith): Invalid parameter number '2' value", - "Ошибка при вызове метода контекста (СтрЗаканчиваетсяНа): Недопустимое значение параметра номер '2'")); + "Ошибка при вызове метода контекста (СтрЗаканчиваетсяНа): Недопустимое значение параметра номер '2'", + "Error calling context method (StrEndsWith): Invalid parameter number '2' value")); }Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/OneScript.StandardLibrary/StringOperations.cs (1)
277-312: Minor: Inconsistent indentation on closing brace.The closing brace on line 312 uses 5 spaces instead of 4 spaces. This is a minor formatting nit.
Otherwise, the exception class design is clean with factory methods providing localized error messages.
🔎 Suggested fix:
$"Template syntax error at position {pos}. Invalid substitution number: '{num}'")); } - } + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/OneScript.StandardLibrary/StringOperations.cs(12 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/StringOperations.cs
🧠 Learnings (3)
📓 Common learnings
Learnt from: EvilBeaver
Repo: EvilBeaver/OneScript PR: 0
File: :0-0
Timestamp: 2024-08-05T18:27:08.563Z
Learning: For each new pull request in the EvilBeaver/OneScript repository, include a poem in Russian.
📚 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/StringOperations.cs
📚 Learning: 2025-09-04T11:15:14.305Z
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/StringOperations.cs
🔇 Additional comments (7)
src/OneScript.StandardLibrary/StringOperations.cs (7)
25-27: Good optimization: precompiled static regex.Using a static compiled
Regexinstance instead of creating a new one per call toStrTemplateis a solid performance improvement. The pattern correctly handles%%escapes,%Nand%(N)substitutions, and stray%markers.
50-64: LGTM!The method correctly validates that
searchStringis non-empty wheninputStringhas content, and uses the new typedStringOpExceptionfor clearer error messaging.
73-87: LGTM!Consistent with
StrStartsWith– validatessearchStringand uses the typed exception.
97-114: LGTM!The null-safe delimiter handling (
stringDelimiter?.ToCharArray()) and use ofArray.Empty<string>()are appropriate. Logic is clean.
122-127: LGTM!Minor formatting adjustments; behavior preserved.
229-252: LGTM!Good use of the shared precompiled regex and typed exceptions for template errors. The error messages include position information which aids debugging.
151-172: Add clarification for asymmetricoccuranceparameter handling or standardize behavior.The method returns
0whenoccurance < 0but throws an exception whenoccurance == 0. This inconsistency should either be documented with a comment explaining why this matches 1C:Enterprise platform design, or the behavior should be standardized (e.g., throw for both invalid cases, or return 0 for both). This impacts the API's predictability for callers.
2 participants
Странноватое поведение
СтрНайтипроверено на последнем выпущенном релизе 8.3.27.1936Summary by CodeRabbit
Bug Fixes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.