[WIP] fix #1679: добавлен метод ОткрытьПоток() объекта ЧтениеJSON#1680
Open
Mr-Rm wants to merge 1 commit into
Open
[WIP] fix #1679: добавлен метод ОткрытьПоток() объекта ЧтениеJSON#1680Mr-Rm wants to merge 1 commit into
Mr-Rm wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughJSONReader adds stream-context support via a new ChangesJSON stream reading support
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/json/test-json_reader.os (1)
225-233: ⚖️ Poor tradeoffConsider expanding test coverage.
The test correctly validates the basic happy path with
ПотокВПамяти(MemoryStreamContext). To improve robustness, consider adding tests for:
- The optional
encodingparameter with non-UTF-8 encodings- Error cases: invalid stream context type, null argument
- Other stream context types (
FileStreamContextorGenericStream)- Stream state after reading (verify
Close()behavior)These additions would help ensure the implementation handles edge cases correctly.
🤖 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 `@tests/json/test-json_reader.os` around lines 225 - 233, Add additional test cases alongside Тест_Должен_ПроверитьОткрытиеПотока to cover edge cases: create a test that passes a ПотокВПамяти with a non-UTF8 encoding and assert correct parsing using ЧтениеJSON and ПрочитатьJSON; add tests that call Чтение.ОткрытьПоток with an invalid stream context type and with null and assert that the right exceptions are raised; add a test using FileStreamContext or GenericStream to ensure file-backed streams parse the same way; and add a test that reads then checks stream state/Close() behavior after ПрочитатьJSON to verify the stream is closed or left open as intended.src/OneScript.StandardLibrary/Json/JSONReader.cs (1)
280-300: ⚡ Quick winAdd exception handling for consistency with
OpenFile.The
OpenFilemethod (lines 252-262) wraps all file and encoding operations in a try-catch block and converts exceptions toRuntimeException. For API consistency,OpenStreamshould follow the same pattern, sinceGetUnderlyingStream(),TextEncodingEnum.GetEncoding(), and theStreamReaderconstructor can all throw exceptions.🔄 Proposed exception handling
public void OpenStream(IValue streamContext, IValue encoding = null) { if (IsOpen()) Close(); + try + { var stream = streamContext switch { GenericStream s => s.GetUnderlyingStream(), FileStreamContext s => s.GetUnderlyingStream(), MemoryStreamContext s => s.GetUnderlyingStream(), _ => throw RuntimeException.InvalidNthArgumentType(1) }; var enc = encoding != null ? TextEncodingEnum.GetEncoding(encoding) : System.Text.Encoding.UTF8; _reader = new JsonReaderInternal(new StreamReader(stream, enc)) { SupportMultipleContent = true }; + } + catch (Exception e) + { + throw new RuntimeException(e.Message, e); + } }🤖 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/Json/JSONReader.cs` around lines 280 - 300, OpenStream lacks the try-catch used in OpenFile, so exceptions from GetUnderlyingStream, TextEncodingEnum.GetEncoding or StreamReader/JsonReaderInternal are not converted to RuntimeException; wrap the body of OpenStream in a try-catch that mirrors OpenFile: catch any Exception and rethrow as RuntimeException (use the same message/format used in OpenFile), while keeping the same behavior of closing an already open reader via IsOpen()/Close() and preserving SupportMultipleContent on JsonReaderInternal.
🤖 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.
Nitpick comments:
In `@src/OneScript.StandardLibrary/Json/JSONReader.cs`:
- Around line 280-300: OpenStream lacks the try-catch used in OpenFile, so
exceptions from GetUnderlyingStream, TextEncodingEnum.GetEncoding or
StreamReader/JsonReaderInternal are not converted to RuntimeException; wrap the
body of OpenStream in a try-catch that mirrors OpenFile: catch any Exception and
rethrow as RuntimeException (use the same message/format used in OpenFile),
while keeping the same behavior of closing an already open reader via
IsOpen()/Close() and preserving SupportMultipleContent on JsonReaderInternal.
In `@tests/json/test-json_reader.os`:
- Around line 225-233: Add additional test cases alongside
Тест_Должен_ПроверитьОткрытиеПотока to cover edge cases: create a test that
passes a ПотокВПамяти with a non-UTF8 encoding and assert correct parsing using
ЧтениеJSON and ПрочитатьJSON; add tests that call Чтение.ОткрытьПоток with an
invalid stream context type and with null and assert that the right exceptions
are raised; add a test using FileStreamContext or GenericStream to ensure
file-backed streams parse the same way; and add a test that reads then checks
stream state/Close() behavior after ПрочитатьJSON to verify the stream is closed
or left open as intended.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a309e50b-9baa-46fc-8be3-1f4f577b6437
📒 Files selected for processing (2)
src/OneScript.StandardLibrary/Json/JSONReader.cstests/json/test-json_reader.os
Collaborator
|
Огонь огонь! |
EvilBeaver
reviewed
May 13, 2026
| if (IsOpen()) | ||
| Close(); | ||
|
|
||
| var stream = streamContext switch |
Owner
There was a problem hiding this comment.
а просто IStreamWrapper не пойдет? Они все его реализуют
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests