Поддержка потока в ЗаписьТекста #1639
Conversation
WalkthroughAdds stream-backed writing to TextWrite: new stream-aware constructors and an Open overload that accepts either a file path or a stream, deferred writer initialization for streams, encoding/BOM resolution for both targets, two new RuntimeException factory methods for stream errors, and expanded tests covering files and streams. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Files/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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
tests/text-write.os (7)
293-300: Stream seek should occur before creating the reader.The
ЧтениеТекстаis created on line 293, butПоток.Перейти(0, ПозицияВПотоке.Начало)is called on line 294 afterward. Depending on howЧтениеТекстаinitializes, it may read from the current (end) position. Move the seek before creating the reader:Suggested fix
// Тогда + Поток.Перейти(0, ПозицияВПотоке.Начало); ЧтениеТекста = Новый ЧтениеТекста(Поток); - Поток.Перейти(0, ПозицияВПотоке.Начало);Additionally, the try-catch block (lines 296-300) only rethrows the exception without any cleanup—it can be removed for clarity.
314-323: Same stream seek ordering issue.The seek on line 316 should occur before creating
ЧтениеТекстаon line 315 to ensure reading from the beginning. The try-catch block is also redundant here.
337-346: Same stream seek ordering issue as above.Move
Поток.Перейти(0, ПозицияВПотоке.Начало)before creatingЧтениеТекста.
361-369: Same stream seek ordering issue.Move the seek before reader creation.
428-434: Stream seek ordering.Line 431 seeks after reader creation at line 429. While this may work if
ЧтениеТекстаdoesn't buffer on construction, it's safer to seek first.
458-464: Stream seek ordering.Same issue—seek at line 461 should precede reader creation at line 459.
519-525: Stream seek ordering in helper.Move line 523 (
Поток.Перейти(0, ПозицияВПотоке.Начало)) before line 521 whereЧтение.Открытьis called, to ensure reading starts from the beginning.Suggested fix
// Тогда + Поток.Перейти(0, ПозицияВПотоке.Начало); Чтение = Новый ЧтениеТекста; Чтение.Открыть(Поток, "UTF-8", Символы.ПС, РазделительСтрокВФайле); - - Поток.Перейти(0, ПозицияВПотоке.Начало); ПроверяемыйТекст = Чтение.Прочитать();
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/OneScript.Core/Exceptions/RuntimeException.cs(1 hunks)src/OneScript.StandardLibrary/Text/TextWriteImpl.cs(4 hunks)tests/text-write.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.Core/Exceptions/RuntimeException.cssrc/OneScript.StandardLibrary/Text/TextWriteImpl.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.Core/Exceptions/RuntimeException.cssrc/OneScript.StandardLibrary/Text/TextWriteImpl.cs
📚 Learning: 2025-12-01T15:13:19.064Z
Learnt from: CR
Repo: EvilBeaver/OneScript PR: 0
File: .cursor/rules/runbsltests.mdc:0-0
Timestamp: 2025-12-01T15:13:19.064Z
Learning: Ignore errors from http.os test file and the test named 'Тест_ДолженПроверитьЧтоПеремещениеФайлаРаботаетПоHTTP' when reviewing BSL test results
Applied to files:
tests/text-write.os
🔇 Additional comments (16)
tests/text-write.os (1)
22-34: Good test coverage for new stream functionality.The new test sections comprehensively cover stream writing scenarios including encoding variations, BOM handling, error cases (closed stream, read-only stream), and switching between file and stream modes.
src/OneScript.Core/Exceptions/RuntimeException.cs (1)
194-206: LGTM!The new
ClosedStream()andNonWritableStream()factory methods follow the established pattern, provide clear bilingual messages, and align with the stream validation requirements inTextWriteImpl.src/OneScript.StandardLibrary/Text/TextWriteImpl.cs (14)
25-29: LGTM!The new
_streamWrapperand_encodingfields appropriately support deferred writer initialization for stream-based writing.
46-49: LGTM!The new stream constructor properly delegates to
Openwith the stream, encoding, and BOM flag parameters.
108-115: LGTM!The
Writemethod now properly callsPrepareForWriting()before writing, ensuring the writer is initialized and stream state is validated for both file and stream targets.
123-132: LGTM!
WriteLinenow properly delegates toWrite, ensuring stream validation and writer preparation occur consistently.
140-149: LGTM!Setting
_streamWrapper = nullwithout disposing the underlying stream is correct since theStreamWriteris created withleaveOpen: true(line 158), allowing the stream to be reused for reading.
151-163: LGTM!The
PrepareForWritingmethod properly validates stream state before creating the writer, and correctly usesleaveOpen: trueto allow stream reuse after writing.
165-176: LGTM!
OpenFileproperly cleans up previous state viaDispose(), sets_streamWrapper = nullto indicate file mode, and creates theStreamWriterdirectly for file-based writing.
178-186: LGTM!
OpenStreamcorrectly stores the stream wrapper and encoding for deferred writer creation inPrepareForWriting, enabling proper stream state validation at write time.
188-202: LGTM!The logic correctly handles the UTF-8 BOM edge case: when appending to an existing file, it uses
UTF8Encoding(false)to prevent writing a duplicate BOM.
204-214: LGTM!Stream encoding resolution properly respects the
writeBomflag for both default UTF-8 and specified encodings.
216-224: LGTM!The property pattern
stream is { CanWrite: false, CanRead: false }is C# 8 compatible and correctly identifies a closed stream.
226-230: LGTM!Simple and effective validation using
IStreamWrapper.IsReadOnlyto prevent writes to read-only streams.
255-261: LGTM!The script constructor factory properly mirrors the updated
Opensignature, enabling both file and stream initialization from scripts.
75-95: No action required. TheConvertValueStrict<T>method handles null parameters gracefully by returning default values (nullfor strings,falsefor booleans) rather than throwing exceptions. The parameter reinterpretation based on whether the first argument is a stream or file is intentional, with each path receiving semantically appropriate parameters for its use case.Likely an incorrect or invalid review comment.
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
||
| private void ThrowIfNonWritableStream() | ||
| { | ||
| if (_streamWrapper != null && _streamWrapper.IsReadOnly) |
There was a problem hiding this comment.
На днях починили поток только для чтения, чтобы сам выбрасывал исключения af962b1
Наверное имеет смысл обновиться с develop и убрать эту проверку, чтобы не дублировать тексты исключения
There was a problem hiding this comment.
Без этого возникнет исключение с родным сообщением от .NET, поскольку запись будет происходить напрямую в Stream из StreamWriter. А с этим подходом текст исключения будет таким же, как в 1С.
There was a problem hiding this comment.
Не очень понял... Мы используем тут не родной стрим .net, а наш враппер стримов. В коммите af962b1 сделана проверка на ReadOnly в наших стримах с текстом исключения, как в 1С.
Или я не прав?
There was a problem hiding this comment.
На данный момент просто реализовано так, что запись (Записать, ЗаписатьСтроку) идёт не через врапер потоков, а через StreamWriter, который работает напрямую с потоком .NET из врапера. Поэтому исключения будут дотнетовские.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/text-write.os(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 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-01T15:13:19.064Z
Learnt from: CR
Repo: EvilBeaver/OneScript PR: 0
File: .cursor/rules/runbsltests.mdc:0-0
Timestamp: 2025-12-01T15:13:19.064Z
Learning: Ignore errors from http.os test file and the test named 'Тест_ДолженПроверитьЧтоПеремещениеФайлаРаботаетПоHTTP' when reviewing BSL test results
Applied to files:
tests/text-write.os
🔇 Additional comments (7)
tests/text-write.os (7)
9-36: LGTM! Clear test organization.The test structure is well-organized with clear sections for file operations, stream operations, and file/stream interoperability. Test names are descriptive and follow the existing convention.
282-355: LGTM! Comprehensive stream operation tests.The stream tests provide good coverage:
- Basic writing to memory stream
- Custom encoding handling
- BOM marker support
- Setting stream via Open method
All tests correctly verify the written content and use appropriate encoding/BOM parameters.
357-366: Test validates deferred initialization pattern.This test confirms that constructing TextWrite with a closed stream doesn't throw immediately, which aligns with the lazy initialization pattern mentioned in the PR description ("Ленивое создание
StreamWriterдля потоков"). The actual exception is tested separately in line 368-379 when attempting to write.
368-393: LGTM! Error handling tests are correct.Both tests properly verify that:
- Writing to a closed stream throws "Ошибка обращения к закрытому потоку"
- Writing to a read-only stream throws "Попытка записи в поток не поддерживающий запись"
The use of
ПроверитьКодСОшибкойis appropriate for exception verification.
395-453: LGTM! Interop tests validate reopening behavior.Both tests correctly verify that TextWrite can be reopened between file and stream targets, with each destination retaining its respective content. The tests include proper cleanup of temporary files.
471-490: LGTM! File delimiter test helper is correct.The helper properly tests delimiter handling by:
- Writing text with specified in-memory and file delimiters
- Reading it back with matching delimiters
- Comparing the results
The parameter order matches the expected signature:
Открыть(ИмяФайла, encoding, lineSeparator, BOM, fileSeparator).
515-532: LGTM! Comparison helpers improve test diagnostics.The helpers correctly escape line breaks (CR and LF) for clearer error messages when delimiter tests fail. This makes debugging much easier by showing exactly which line endings are present.
2 participants
Добавляет поддержку потока в
ЗаписьТекста(#1532)Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Note
Extends
TextWriterto write to streams with configurable encoding/BOM and line delimiters, adds errors for closed/read-only streams, and updates tests accordingly.TextWriteImpl:IValue/IStreamWrapperinOpenand constructors (file or stream).ResolveEncodingForFile/Stream).StreamWriterfor streams (PrepareForWriting), with immediate auto-flush.RuntimeException.ClosedStream()andRuntimeException.NonWritableStream()factory methods.Written by Cursor Bugbot for commit cebce69. This will update automatically on new commits. Configure here.