fix(java): rewrite literalEval() with recursive descent parser#252
Conversation
Fix ControlCore-Project#228: concoredocker.java literalEval() is fundamentally broken The original implementation used String.split(",") which failed for: - Nested structures (lists within dicts, etc.) - Strings containing commas or colons - Escape sequences in strings Changes: - Implement recursive descent parser for Python literal syntax - Add escapePythonString() for proper string escaping - Fix toPythonLiteral() to use Python True/False/None - Fix read() to return List<Object> with max retries - Fix write() delay to double (was int) with separate catch blocks - Fix simtime to double type - Add TestLiteralEval.java with 49 comprehensive tests All tests pass.
|
Hey @pradeeban CI Failures NoteThe CI failures are not caused by this PR's Java changes - they are pre-existing issues in the \dev\ branch:
The Java changes in this PR are correct and all 49 Java tests pass locally. These CI issues exist independently of this PR and would need to be fixed in a separate PR targeting the CI/Python files. |
There was a problem hiding this comment.
Pull request overview
This PR fixes the Java concoredocker implementation by replacing the previously broken literalEval() behavior with a recursive-descent parser for Python-style literals, and updates related I/O handling to better match the Python runtime behavior.
Changes:
- Replaced
literalEval()with a recursive descent parser supporting nested dict/list/tuple, numbers, strings, booleans, andNone. - Updated read/write helpers (retry behavior, delay handling,
simtimeasdouble) and added Python-literal serialization helpers. - Added a standalone Java test program
TestLiteralEval.javato exercise the new parser.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| concoredocker.java | Implements the new literal parser, updates simtime/delay handling, and revises read/write parsing/serialization behavior. |
| TestLiteralEval.java | Adds a standalone test runner validating core parsing behaviors and a subset of edge cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Update: Created PR #253 to fix the CI dependency issues separately. Once that PR is merged, the CI should pass for this Java-only PR. |
- Remove s += initstr on max-retries and parse-error paths in read() (accumulator should not include default when returning it) - Add dict key validation: throw if key is not a non-null String - Use explicit UTF-8 charset in parseFile() and sparams reading - Align sparams parsing with Python parse_params logic: try literalEval first if input looks like dict literal - Update test comment to reflect actual coverage - Remove unused list creation in testRoundTripSerialization() - Add test for non-string dict key validation All 50 tests pass.
|
Thanks, merged both PRs now. |
|
thank you so much @pradeeban its really boosting my confidence.... |
This PR fixes #228 related to the
literalEval()implementation inconcoredocker.java.The previous implementation was using
String.split(","), which caused problems in several situations such as nested structures (like lists inside dictionaries), strings containing commas or colons, and cases with escape sequences. Because of this, some valid Python literals were not being parsed correctly.To fix this, I replaced the logic with a recursive descent parser that can properly handle Python literal syntax. I also added an
escapePythonString()method for correct string escaping and updatedtoPythonLiteral()so it uses Python values likeTrue,False, andNone.Some additional fixes were also made:
read()now returnsList<Object>and includes a retry mechanismwrite()delay is changed todoubleinstead ofint, with separate catch blockssimtimetype is updated todoubleI also added a test file
TestLiteralEval.javawith 49 test cases to verify different scenarios.All 49 tests pass locally.
This PR contains only the Java files as requested:
concoredocker.java(fixed implementation)TestLiteralEval.java(test suite)Fixes #228