Rtop 38 add support for array on debug.c #11
Conversation
WalkthroughRefactors ComplexParser into a stateful, class-based, block-oriented multi-pass parser with templates; adds STParser for regex-based ST block/type detection; adds a CSV variable Jinja2 template; updates xml2st to use ComplexParser in-place for ST/CSV changes; minor devcontainer config added. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI as xml2st CLI
participant CP as ComplexParser
participant Gen as Debugger Generator
rect rgb(245,248,255)
note over CLI: --generate-st <st_file>
User->>CLI: invoke
CLI->>CP: RewriteST(st_file)
CP-->>CLI: ST rewritten in-place
end
rect rgb(245,255,245)
note over CLI: --generate-debug <st_file> <csv_file>
User->>CLI: invoke
CLI->>CP: AddComplexVars(st_file, csv_file)
CP-->>CLI: CSV augmented in-place
CLI->>Gen: Generate_embedded_plc_debugger(st_file)
Gen-->>CLI: (cfg, debug_text)
CLI-->>User: debug_text
end
sequenceDiagram
autonumber
participant CP as ComplexParser
participant STP as STParser
participant Tpl as Templates
CP->>CP: __clear()
CP->>STP: _parseStTree(lines)
STP-->>CP: Block tree (arrays/structs/vars/named blocks)
CP->>CP: __analyseTypes() / __separateOuterBlocks()
CP->>Tpl: Render FUNCTION_BLOCK_ST_TEMPLATE (if required)
CP->>CP: __getSTLines() → __rewriteSTWithComplexStructs()
CP->>CP: __findProgramInstances() / __spreadDeclarations()
CP->>Tpl: Render CSV_VARS_TEMPLATE for vars
CP-->>CP: __appendVarsToCSV()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (2 warnings, 1 inconclusive)❌ Failed checks (2 warnings, 1 inconclusive)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
✨ Finishing Touches
🧪 Generate unit tests
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: 4
🔭 Outside diff range comments (1)
xml2st.py (1)
61-75: Theparse_complex_variablesfunction is no longer needed.This function is not called anywhere in the codebase and its functionality has been replaced by the
ComplexParser.RewriteSTmethod used in lines 132-133.Consider removing this unused function:
-def parse_complex_variables(st_file): - if not os.path.isfile(st_file) or not st_file.lower().endswith(".st"): - print( - f"Error: Invalid file '{st_file}'. A path to a st file is expected.", - file=sys.stderr, - ) - return None - - parser = ComplexParser() - try: - parser.ParseSTFile(st_file) - except Exception as e: - print(f"Error parsing ST file: {e}", file=sys.stderr) - sys.exit(1) - -
🧹 Nitpick comments (6)
templates/variable_declaration.csv.j2 (1)
1-1: Consider adding a newline at the end of the template file.Template files typically end with a newline character for better compatibility with various text processing tools and to follow Unix conventions.
-{{i}};VAR;{{var["name"]}};{{var["name"]}};{{var["type"]}}; +{{i}};VAR;{{var["name"]}};{{var["name"]}};{{var["type"]}}; +STParser.py (1)
149-151: Consider encapsulating the RESOURCE regex reassignment.Directly reassigning the
startattribute after instantiation breaks encapsulation and could lead to maintenance issues. Consider creating a specialized_ResourceBlockclass instead.+class _ResourceBlock(_NamedBlock): + def __init__(self): + super().__init__("resource") + self.start = re.compile( + rf"^\s*{self.name}\s+(?P<name>[A-Za-z_][A-Za-z0-9_]*)\s+ON\s+PLC\s*$" + ) + TYPE = _Block("type") FUNCTION_BLOCK = _NamedBlock("function_block") PROGRAM = _NamedBlock("program") CONFIGURATION = _NamedBlock("configuration") -RESOURCE = _NamedBlock("resource") -RESOURCE.start = re.compile( - rf"^\s*{RESOURCE.name}\s+(?P<name>[A-Za-z_][A-Za-z0-9_]*)\s+ON\s+PLC\s*$" -) +RESOURCE = _ResourceBlock() STRUCT = _StructBlock("struct")ComplexParser.py (4)
1-1: Separate imports on different lines for better readability.PEP 8 recommends placing each import on a separate line.
-import os, re +import os +import re
196-196: Simplify dictionary key check.Using
key in dictis more pythonic thankey in dict.keys().- elif "name" in info.keys(): + elif "name" in info:
312-312: Use a more descriptive variable name.Single-letter variable names like
lare ambiguous and can be confused with the number1.- lines = [l.strip() for l in self.__getBlockLines(struct)[1:-1]] + lines = [line.strip() for line in self.__getBlockLines(struct)[1:-1]]
439-443: Avoid writing to file and returning the same content.The method writes to the file and also returns the same content, which seems redundant.
Either return the content without writing:
- with open(csv_file, "w") as f: - f.writelines( - content[:ticktime_idx] + formatted_vars + [""] + content[ticktime_idx:] - ) - - return content[:ticktime_idx] + formatted_vars + [""] + content[ticktime_idx:] + updated_content = content[:ticktime_idx] + formatted_vars + [""] + content[ticktime_idx:] + with open(csv_file, "w") as f: + f.writelines(updated_content) + return updated_contentOr remove the return statement if the return value is not used.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ComplexParser.py(1 hunks)STParser.py(1 hunks)templates/variable_declaration.csv.j2(1 hunks)xml2st.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
ComplexParser.py
4-4: from STParser import * used; unable to detect undefined names
(F403)
67-67: VARIABLE may be undefined, or defined from star imports
(F405)
71-71: BASE_TYPES may be undefined, or defined from star imports
(F405)
92-92: ARRAY may be undefined, or defined from star imports
(F405)
102-102: STRUCT may be undefined, or defined from star imports
(F405)
181-181: ARRAY may be undefined, or defined from star imports
(F405)
187-187: STRUCT may be undefined, or defined from star imports
(F405)
191-191: VARIABLE may be undefined, or defined from star imports
(F405)
196-196: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
262-262: TYPE may be undefined, or defined from star imports
(F405)
264-264: FUNCTION_BLOCK may be undefined, or defined from star imports
(F405)
267-267: PROGRAM may be undefined, or defined from star imports
(F405)
279-279: ALL_BLOCKS may be undefined, or defined from star imports
(F405)
283-283: CLOSABLE_BLOCKS may be undefined, or defined from star imports
(F405)
298-298: STRUCT may be undefined, or defined from star imports
(F405)
298-298: block may be undefined, or defined from star imports
(F405)
312-312: Ambiguous variable name: l
(E741)
341-341: ARRAY may be undefined, or defined from star imports
(F405)
344-344: BASE_TYPES may be undefined, or defined from star imports
(F405)
352-352: VARIABLE may be undefined, or defined from star imports
(F405)
354-354: BASE_TYPES may be undefined, or defined from star imports
(F405)
367-367: STRUCT may be undefined, or defined from star imports
(F405)
384-384: CONFIGURATION may be undefined, or defined from star imports
(F405)
386-386: RESOURCE may be undefined, or defined from star imports
(F405)
389-389: PROGRAM_DEFINITION may be undefined, or defined from star imports
(F405)
391-391: PROGRAM_DEFINITION may be undefined, or defined from star imports
(F405)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-windows-arm
🔇 Additional comments (1)
ComplexParser.py (1)
419-420: Add bounds check before adjusting ticktime_idx.The while loop could potentially set
ticktime_idxto 0, makingticktime_idx - 1negative in line 425-427.while ticktime_idx >= 1 and EMPTY_LINE.match(content[ticktime_idx - 1]): ticktime_idx -= 1 last_var_number = -1 if ticktime_idx > 0: - extracted_var_number = var_number_pattern.match( - content[ticktime_idx - 1] - ).group(1) + match = var_number_pattern.match(content[ticktime_idx - 1]) + if match: + extracted_var_number = match.group(1) - if extracted_var_number: - last_var_number = int(extracted_var_number) + if extracted_var_number: + last_var_number = int(extracted_var_number)Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ComplexParser.py (1)
303-315: Fixed reference to correct loop variable.The original code referenced an undefined
blockvariable instead of the loop variableb. This is now corrected.
🧹 Nitpick comments (3)
ComplexParser.py (3)
19-24: Fix typo in comment.-## GLOABL VARIABLES +## GLOBAL VARIABLES
209-209: Use direct dictionary key membership check.- elif "name" in info.keys(): + elif "name" in info:
325-325: Use more descriptive variable name.- lines = [l.strip() for l in self.__getBlockLines(struct)[1:-1]] + lines = [line.strip() for line in self.__getBlockLines(struct)[1:-1]]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ComplexParser.py(1 hunks)STParser.py(1 hunks)xml2st.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
ComplexParser.py
209-209: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
325-325: Ambiguous variable name: l
(E741)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-windows-arm
🔇 Additional comments (37)
STParser.py (8)
1-13: LGTM! Well-defined base types constants.The BASE_TYPES list includes standard IEC 61131-3 data types commonly used in PLC programming. The implementation is clean and appropriate.
16-29: LGTM! Clean base block detector implementation.The
_Blockclass provides a solid foundation for block detection with proper regex compilation and a clear GetInfo interface.
31-46: LGTM! Proper extension of base block for named blocks.The
_NamedBlockclass correctly extends_Blockand adds named group capture for block names. The regex pattern properly validates identifier naming conventions.
48-55: LGTM! Appropriate specialization for struct blocks.The
_StructBlockclass correctly handles the different syntax for struct definitions where the name appears before the colon, and properly terminates with a semicolon.
57-80: LGTM! Comprehensive variable declaration parsing.The
_DataTypeclass properly handles variable declarations with optional initialization values. The regex pattern correctly captures all components and ensures proper line termination.
82-104: LGTM! Array declaration parsing handles negative indices correctly.The
_ArrayTypeclass properly extends_DataTypeand handles array syntax with support for negative start indices. The regex pattern correctly captures array bounds and element types.
106-126: LGTM! Program definition parsing is well-structured.The
_ProgramDefinitionclass correctly parses the PROGRAM...WITH...syntax and extracts all necessary components (instance, task, program name).
128-159: LGTM! Well-organized detector instances and collections.The module-level instances and collections (ALL_BLOCKS, CLOSABLE_BLOCKS) provide a clean API for the ComplexParser to use. The RESOURCE specialization for "ON PLC" syntax is appropriate.
xml2st.py (3)
8-8: LGTM! Updated import aligns with new architecture.The import of ComplexParser is consistent with the refactored parsing approach described in the AI summary.
116-117: LGTM! Proper usage of new ComplexParser API.The code correctly uses the new
RewriteSTmethod instead of the removedparse_complex_variablesfunction, aligning with the new class-based architecture.
125-128: LGTM! Correct implementation of complex variable handling.The code properly uses the new
AddComplexVarsmethod with both ST file and CSV file arguments, which replaces the previous parsing approach.ComplexParser.py (26)
4-17: LGTM! Explicit imports resolve wildcard import issues.The explicit imports from STParser are much clearer than the previous wildcard import and resolve the static analysis warnings about undefined names.
28-31: LGTM! Simple helper class for line insertion tracking.The
_InsertLineclass provides a clean way to track where inner blocks should be inserted during line reconstruction.
33-70: LGTM! Well-structured hierarchical block representation.The
_BlockInstanceclass provides a solid foundation for representing nested block structures with proper state management for opened/closed blocks.
72-76: LGTM! Clean extension for named blocks.The
_NamedBlockInstanceclass appropriately extends the base block to add name tracking.
78-100: LGTM! Comprehensive variable instance implementation.The
_VariableInstanceclass properly handles variable declarations with data types, values, and simplicity tracking for type analysis.
102-110: LGTM! Array instance extends variable appropriately.The
_ArrayInstanceclass correctly extends_VariableInstanceand adds array-specific properties for start/end indices.
111-125: LGTM! Struct instance with proper simplicity propagation.The
_StructInstanceclass correctly tracks whether the struct contains only simple types by propagating simplicity from inner blocks.
133-146: LGTM! Well-initialized parser state.The constructor properly initializes all necessary collections for tracking different types of blocks and variables.
147-160: LGTM! Complete state reset functionality.The
__clearmethod properly resets all parser state, enabling reuse of the parser instance.
161-168: LGTM! Proper block closing with error handling.The
__closemethod includes appropriate error handling for attempting to close blocks when none are open.
169-177: LGTM! Correct block hierarchy management.The
__appendBlockmethod properly manages the block hierarchy by adding to the current open block or the top level.
178-189: LGTM! Line appending with empty line handling.The
__appendLinemethod correctly handles empty lines and ensures lines are added to the appropriate block context.
190-213: LGTM! Comprehensive block classification.The
__classifyBlockmethod properly creates appropriate instance types based on the detected block information and maintains the necessary collections.
214-227: LGTM! Recursive line extraction from blocks.The
__getBlockLinesmethod correctly reconstructs lines from the hierarchical block structure, handling nested insertions appropriately.
228-268: LGTM! Sophisticated type analysis algorithm.The
__analyseTypesmethod implements a proper iterative algorithm to resolve type dependencies and correctly categorize simple vs complex types.
269-282: LGTM! Proper separation of different block types.The
__separateOuterBlocksmethod correctly categorizes top-level blocks into their appropriate collections for further processing.
283-302: LGTM! Main parsing logic is well-structured.The
_parseStTreemethod implements a clean parsing loop that processes each line through the detector chain and builds the block hierarchy.
316-331: LGTM! Template-based ST file rewriting.The method properly uses Jinja2 templates to generate function block definitions and rewrites the ST file with the processed content.
332-349: LGTM! Helper methods for type and function block lookup.Both
__getCustomTypeand__getFunctionBlockprovide clean lookup functionality for the declaration spreading logic.
350-384: LGTM! Comprehensive declaration spreading algorithm.The
__spreadDeclarationsmethod correctly handles arrays, variables, and structs with proper prefix building and recursive type resolution.
385-391: LGTM! Program variable declaration extraction.The method correctly finds program blocks and extracts their variable declarations for CSV processing.
392-409: LGTM! Program instance discovery in configuration blocks.The method properly traverses the configuration/resource hierarchy to find program definitions and build appropriate prefixes.
410-457: LGTM! Robust CSV manipulation with proper error handling.The
__appendVarsToCSVmethod correctly locates the Ticktime section, handles variable numbering, and inserts new variables while preserving file structure.
458-462: LGTM! Simple orchestration of CSV rewriting.The method properly coordinates program instance discovery and CSV variable appending.
465-479: LGTM! Complete complex variable addition workflow.The
AddComplexVarsmethod provides proper file validation, state management, and orchestrates the full process of parsing ST and updating CSV files.
480-493: LGTM! ST rewriting with proper validation and state management.The
RewriteSTmethod correctly validates inputs, manages parser state, and performs the ST file rewriting with complex structs.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/build.yml (1)
36-37: Optional: DRY the build steps and handle platform-specific add-data separators via a matrix.You can reduce duplication and avoid Windows-specific quoting pitfalls by moving to a single matrix job with an os-specific add-data separator variable (":" vs ";") and a shell override for Windows if needed. This makes future changes (e.g., adding another data dir) a single-line edit.
If you want, I can draft a matrix-based workflow that sets ADD_DATA_SEP per runner and constructs a single PyInstaller command accordingly.
Also applies to: 64-65, 97-98, 130-131, 158-159, 186-187
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build.yml(6 hunks)
🔇 Additional comments (6)
.github/workflows/build.yml (6)
37-37: LGTM: Packaging the entire templates directory for macOS ARM is correct.The switch to "--add-data templates:templates" aligns with the new template-driven pipeline. No issues here.
65-65: LGTM: macOS x64 build now bundles templates directory.The add-data usage is correct for non-Windows platforms.
98-98: LGTM: Linux ARM one-file build includes templates directory.The colon separator is correct for Linux.
131-131: LGTM: Linux x64 one-file build includes templates directory.This matches the new runtime template lookup.
36-37: Verified: Required data directories are present
Thetemplates/andplcopen/directories exist at the repository root and contain the expected files, so PyInstaller’s--add-data plcopen:plcopen --add-data templates:templatesinvocation will succeed. No further changes are needed.
23-27: Runner labels confirmed valid; note public-only Windows ARM and upcoming macOS deprecationThe
runs-onlabels in .github/workflows/build.yml (lines 18, 46, 74, 107, 140, 168) have been verified against GitHub’s hosted runners:
- macos-15 – valid
- macos-13 – valid but scheduled for deprecation by Nov 14 2025; plan to migrate to macos-14 or macos-15
- ubuntu-22.04-arm & ubuntu-22.04 – both valid for ARM64 and x64 environments
- windows-11-arm – valid only for public repositories; private repos require a self-hosted runner with this label or to switch to Windows Server labels
- windows-2025 – valid (windows-latest will migrate to Server 2025 by Sept 30 2025)
No further changes required—just confirm your repo’s visibility and runner setup for windows-11-arm.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
xml2st.py (2)
61-72: Return type inconsistency and missing validation for st_fileThe success path returns a string, but the error path returns a tuple (None, None). This will break callers that expect a string. Also, validate st_file early.
Apply this diff:
def generate_debugger_file(csv_file, st_file): if not os.path.isfile(csv_file) or not csv_file.lower().endswith(".csv"): print( f"Error: Invalid file '{csv_file}'. A path to a csv file is expected.", file=sys.stderr, ) - return None, None + return None + if not os.path.isfile(st_file) or not st_file.lower().endswith(".st"): + print( + f"Error: Invalid file '{st_file}'. A path to a st file is expected.", + file=sys.stderr, + ) + return None controler = ProjectController() controler.SetCSVFile(csv_file) return controler.Generate_embedded_plc_debugger(st_file)[1]
61-72: Fix inconsistent return type and caller handling for generate_debugger_fileSummary: generate_debugger_file(csv_file, st_file) (xml2st.py:61-72) returns (None, None) on error but a string on success. Ripgrep shows the only caller is xml2st.py:129 (single-variable assignment). This mismatch can cause runtime errors (append_debugger_to_st calls .split on None).
Files / locations to fix:
- xml2st.py:61-72 — change the error return to a single None (or raise) so the function always returns a single value.
- Replace:
- return None, None
- With:
- return None
- xml2st.py:127-135 (call site) — handle a None result before passing to append_debugger_to_st:
- After:
- debug_text = generate_debugger_file(args.generate_debug[1], args.generate_debug[0])
- Add:
- if debug_text is None:
print("Error: Failed to generate debugger text", file=sys.stderr)
sys.exit(1)- append_debugger_to_st(args.generate_debug[0], debug_text)
Verification: ripgrep found only the usage in xml2st.py and no callers expecting two return values.
🧹 Nitpick comments (1)
ProjectController.py (1)
176-187: Comment mismatch: MD5 not actually added to debug.c fileYou write debug_text to cfile (Line 177) and only prepend MD5 to the in-memory c_debug string. The comment “Add MD5 value to debug.cpp file” is misleading since the file on disk doesn’t include the md5 symbol.
- If the intent is to embed MD5 only in the ST comments, update the comment accordingly.
- If you also need MD5 in the compiled debug.c, write c_debug to cfile instead of debug_text (be aware of potential multiple-definition/linkage if compiled in multiple TUs).
Example (if you truly want MD5 in debug.c):
- with open(cfile, "w") as f: - f.write(debug_text) + with open(cfile, "w") as f: + f.write(c_debug)Otherwise, just change the comment to “Add MD5 value to the embedded debugger content”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
ProjectController.py(3 hunks)xml2st.py(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
xml2st.py (2)
ProjectController.py (1)
Generate_embedded_plc_debugger(162-187)ComplexParser.py (3)
ComplexParser(129-492)RewriteST(480-492)AddComplexVars(465-478)
🪛 Ruff (0.12.2)
ProjectController.py
180-180: Use a context manager for opening files
(SIM115)
182-182: Cannot raise a literal. Did you intend to return it or raise an Exception?
(B016)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-darwin-x64
- GitHub Check: build-windows-arm
🔇 Additional comments (3)
ProjectController.py (1)
162-175: No action needed — callers updated to new signature and return valueVerified the repo: Generate_embedded_plc_debugger(st_file) now returns (cfile, c_debug) with an MD5-prepended debug string, and the only caller was updated to handle that.
- ProjectController.py (≈lines 162–175): Generate_embedded_plc_debugger(st_file) — returns (cfile, c_debug) where c_debug includes the MD5.
- xml2st.py (line 71): return controler.Generate_embedded_plc_debugger(st_file)[1] — passes st_file and extracts c_debug.
- xml2st.py (append_debugger_to_st): consumes the returned string and appends it to the ST file.
No further call sites found; no fixes required.
xml2st.py (2)
124-131: Good: Correct argument order for AddComplexVars and generate_debugger_file; add guard for NoneArgument order is correct (ST_FILE, CSV_FILE) for AddComplexVars and (CSV_FILE, ST_FILE) for generate_debugger_file. After standardizing generate_debugger_file to return None on error, guard before appending.
Apply this diff:
debug_text = generate_debugger_file( args.generate_debug[1], args.generate_debug[0] ) + if not debug_text: + raise Exception("Failed to generate debugger content.") append_debugger_to_st(args.generate_debug[0], debug_text)
115-117: Incorrect — RewriteST already persists the ST fileComplexParser.RewriteST calls __rewriteSTWithComplexStructs(), which opens self.__stFile and writes the rewritten content (so the file is persisted inside the method). The return value is optional; no extra write is required.
- ComplexParser.py — __rewriteSTWithComplexStructs writes to the file via: with open(self.__stFile, "w") as f: f.write(program_text + "".join(self.__getSTLines()))
- xml2st.py — lines ~115–116: complex_parser = ComplexParser(); complex_parser.RewriteST(st_file)
Remove/ignore the proposed diff — no fix required.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.devcontainer/devcontainer.json (1)
27-27: Align container and remote user to prevent permission mismatches
remoteUsercontrols the user VS Code connects as, but if the container itself runs as a different user (often root), you can hit file permission issues and inconsistent ownership on volumes. Explicitly setcontainerUserto match.Apply this diff to set the container runtime user:
- "remoteUser": "dev", + "remoteUser": "dev", + "containerUser": "dev",Please also verify the Dockerfile creates the
devuser and sets its home at/home/devto match your SSH mount.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
.devcontainer/devcontainer.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-windows-arm
- GitHub Check: build-linux-arm
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (14)
ComplexParser.py (14)
19-24: Typo in section header."GLOABL" ➜ "GLOBAL".
Apply this diff:
-## GLOABL VARIABLES +## GLOBAL VARIABLES
35-41: Avoid shadowing built-intype.Using
typeas a parameter name and local variable can confuse readers and tools. Preferblock_type.Apply this diff:
-class _BlockInstance: - - def __init__(self, type): +class _BlockInstance: + + def __init__(self, block_type): self.inner_blocks = [] self.lines = [] self.opened = True - self.type = type + self.type = block_type-class _NamedBlockInstance(_BlockInstance): - def __init__(self, type, name): - super().__init__(type) +class _NamedBlockInstance(_BlockInstance): + def __init__(self, block_type, name): + super().__init__(block_type) self.name = nameAlso applies to: 73-76
58-66: Incorrect/misleading docstring for AppendLine.Docstring says "Add a block…" but this method appends a line (or delegates to the last opened inner block).
Apply this diff:
def AppendLine(self, line): """ - Add a block to the inner blocks. + Append a line to the current block (or to the last opened inner block). """
209-213: Usein dictinstead ofin dict.keys().Simpler and faster; also addresses Ruff SIM118.
Apply this diff:
- elif "name" in info.keys(): + elif "name" in info: return _NamedBlockInstance(info["type"], info["name"])
291-299: Avoid double-callingGetInfoand simplify closable check.Current code calls
t.GetInfo(line)twice per token and builds a generator that repeats work. Refactor for clarity and efficiency.Apply this diff:
- for line in lines: - info = next((t.GetInfo(line) for t in ALL_BLOCKS if t.GetInfo(line)), None) - if info: - block = self.__classifyBlock(info) - self.__appendBlock(block) - elif next((True for t in CLOSABLE_BLOCKS if t.end.match(line)), False): - self.__close(line) - continue - self.__appendLine(line) + for line in lines: + info = None + for t in ALL_BLOCKS: + info = t.GetInfo(line) + if info: + break + if info: + block = self.__classifyBlock(info) + self.__appendBlock(block) + elif any(t.end.match(line) for t in CLOSABLE_BLOCKS): + self.__close(line) + continue + self.__appendLine(line)
324-326: Rename loop variablelto a non-ambiguous name.Addresses Ruff E741 and improves readability.
Apply this diff:
- lines = [l.strip() for l in self.__getBlockLines(struct)[1:-1]] + lines = [line.strip() for line in self.__getBlockLines(struct)[1:-1]]
401-405: Avoid recomputingPROGRAM_DEFINITION.GetInfo(line)twice.Minor efficiency/readability improvement.
Apply this diff:
- program_instances = [ - PROGRAM_DEFINITION.GetInfo(line) - for line in resource.lines - if PROGRAM_DEFINITION.GetInfo(line) - ] + program_instances = [] + for line in resource.lines: + info = PROGRAM_DEFINITION.GetInfo(line) + if info: + program_instances.append(info)
470-472: Error message refers to ST file when validating CSV file.Copy/paste oversight in the exception message.
Apply this diff:
- if not csv_file or not os.path.isfile(csv_file): - raise Exception("ST file not valid. Please provide a valid CSV file path.") + if not csv_file or not os.path.isfile(csv_file): + raise Exception("CSV file not valid. Please provide a valid CSV file path.")
333-339: Docstring mismatch: returning fromsimple_types, not "complex types".Clarify to avoid confusion during maintenance.
Apply this diff:
- """ - Get the complex type by its name. - """ + """ + Get a previously resolved custom/simple type by its name. + """
273-274: Unused attributeself.custom_types.
self.custom_typesis initialized but never used elsewhere. Remove it or wire it into lookups if planned.Would you like me to provide a follow-up patch to remove it and adjust type lookups accordingly?
452-454: CSV spacer line is an empty string, not a newline.
[""]inserts an empty string without a newline; if the consumer expects a blank line, prefer["\n"]to guarantee line termination.Apply this diff:
- f.writelines( - content[:ticktime_idx] + formatted_vars + [""] + content[ticktime_idx:] - ) + f.writelines( + content[:ticktime_idx] + formatted_vars + ["\n"] + content[ticktime_idx:] + )If the CSV consumer is strict about record counts, confirm whether this extra blank line is desired.
381-384: Narrowisinstancechecks to include arrays explicitly (optional).Given
_ArrayInstancesubclasses_VariableInstance, the current checks work. If you ever introduce another subclass, consider explicit branching or comments to make intent clear.I can add comments to the two
isinstance(..., _VariableInstance)sites for future readers—say the word.Also applies to: 389-391
316-330: Template environment creation per call.Minor: constructing a new Jinja Environment every rewrite is fine, but you already have
self.__loader; cachingEnvironment(loader=self.__loader)in__init__avoids repeated instantiation.Happy to draft a small patch if you want this change.
465-493: API surface looks solid; please add tests for ARRAY expansion paths.Now that arrays are supported, add tests covering:
- Non-zero-based bounds (e.g.,
[3..5]).- Arrays of custom simple types.
- Arrays of complex struct types (ensure behavior is as intended).
I can scaffold pytest cases and a sample ST/CSV fixture set if you’d like.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
ComplexParser.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ComplexParser.py (2)
util/paths.py (1)
AbsDir(36-37)STParser.py (5)
GetInfo(22-28)GetInfo(38-45)GetInfo(70-79)GetInfo(93-103)GetInfo(116-125)
🪛 Ruff (0.12.2)
ComplexParser.py
209-209: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
325-325: Ambiguous variable name: l
(E741)
🔇 Additional comments (1)
ComplexParser.py (1)
4-17: Good fix: explicit imports instead of wildcard.Switching from
from STParser import *to explicit imports improves readability and resolves the Ruff F403/F405 warnings. No further action needed here.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
ComplexParser.py (2)
374-392: Fix array index expansion to honor declared bounds (3..5, etc.).Indices are emitted 0-based, misaligning CSV/debug paths when bounds aren’t 1-based. Emit declared indices.
- if block.type == ARRAY.name: + if block.type == ARRAY.name: array_prefix = prefix if not raw_type: array_prefix = f"{array_prefix}.{block.name.upper()}" if not value_added: array_prefix = f"{array_prefix}.value" - for i in range(0, block.end + 1 - block.start): - indexed_prefix = f"{array_prefix}.table[{i}]" + for idx in range(block.start, block.end + 1): + indexed_prefix = f"{array_prefix}.table[{idx}]" if block.data_type in BASE_TYPES: self.csv_vars.append( {"name": indexed_prefix, "type": block.data_type} ) else: - type = self.__getCustomType(block.data_type) - if type: + ctype = self.__getCustomType(block.data_type) + if ctype: self.__spreadDeclarations( - type, prefix=indexed_prefix, value_added=True + ctype, prefix=indexed_prefix, value_added=True )Consider adding a unit test for ARRAY[3..5] to assert .table[3], .table[4], .table[5] are produced.
473-479: Robustness: guard match() before .group(1).This raises if the previous line isn’t a numbered var; fall back safely.
- if ticktime_idx > 0: - extracted_var_number = var_number_pattern.match( - content[ticktime_idx - 1] - ).group(1) - if extracted_var_number: - last_var_number = int(extracted_var_number) + if ticktime_idx > 0: + m = var_number_pattern.match(content[ticktime_idx - 1]) + if m: + last_var_number = int(m.group(1))
🧹 Nitpick comments (5)
ComplexParser.py (5)
1-1: Style nit: separate stdlib imports.Prefer one import per line for readability and to match common style guides.
Apply:
-import os, re +import os +import re
19-21: Typo in section header.Fix “GLOABL” → “GLOBAL”.
-## GLOABL VARIABLES +## GLOBAL VARIABLES
295-304: Avoid double GetInfo calls and simplify closable check.Current code calls t.GetInfo(line) twice per detector and uses an unnecessary generator for closables.
- for line in lines: - info = next((t.GetInfo(line) for t in ALL_BLOCKS if t.GetInfo(line)), None) + for line in lines: + info = None + for t in ALL_BLOCKS: + info = t.GetInfo(line) + if info: + break if info: block = self.__classifyBlock(info) self.__appendBlock(block) - elif next((True for t in CLOSABLE_BLOCKS if t.end.match(line)), False): + elif any(t.end.match(line) for t in CLOSABLE_BLOCKS): self.__close(line) continue self.__appendLine(line)
330-333: Template environment: enable autoescape and reuse a single Environment.Reduce risk and duplication by constructing one Environment with select_autoescape and reusing it.
- template = Environment(loader=self.__loader).get_template( + template = self.__env.get_template( FUNCTION_BLOCK_ST_TEMPLATE ) @@ - template = Environment(loader=self.__loader).get_template(CSV_VARS_TEMPLATE) + template = self.__env.get_template(CSV_VARS_TEMPLATE)Add in init (outside the shown ranges):
from jinja2 import Environment, FileSystemLoader, select_autoescape self.__env = Environment( loader=self.__loader, autoescape=select_autoescape(enabled_extensions=("j2",), default=False) )Also applies to: 480-481
335-337: Rename loop variable ‘l’ to avoid ambiguity.Minor readability improvement.
- lines = [l.strip() for l in self.__getBlockLines(struct, False)[1:-1]] + lines = [line.strip() for line in self.__getBlockLines(struct, False)[1:-1]]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
ComplexParser.py(1 hunks)STParser.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- STParser.py
🧰 Additional context used
🧬 Code graph analysis (1)
ComplexParser.py (2)
util/paths.py (1)
AbsDir(36-37)STParser.py (5)
GetInfo(22-28)GetInfo(38-45)GetInfo(70-79)GetInfo(94-104)GetInfo(117-126)
🪛 Ruff (0.12.2)
ComplexParser.py
166-166: Create your own exception
(TRY002)
166-166: Avoid specifying long messages outside the exception class
(TRY003)
186-186: Create your own exception
(TRY002)
186-186: Avoid specifying long messages outside the exception class
(TRY003)
330-330: By default, jinja2 sets autoescape to False. Consider using autoescape=True or the select_autoescape function to mitigate XSS vulnerabilities.
(S701)
335-335: Ambiguous variable name: l
(E741)
466-466: Avoid specifying long messages outside the exception class
(TRY003)
480-480: By default, jinja2 sets autoescape to False. Consider using autoescape=True or the select_autoescape function to mitigate XSS vulnerabilities.
(S701)
504-504: Create your own exception
(TRY002)
504-504: Avoid specifying long messages outside the exception class
(TRY003)
507-507: Create your own exception
(TRY002)
507-507: Avoid specifying long messages outside the exception class
(TRY003)
521-521: Create your own exception
(TRY002)
521-521: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (1)
ComplexParser.py (1)
4-17: LGTM: Replaced wildcard import with explicit names.This resolves namespace pollution and improves static analysis.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
xml2st.py (2)
62-73: Fix inconsistent return type in generate_debugger_file (crashes when CSV invalid).On error you return a tuple
(None, None), but on success you return a single string. The caller treats the result as a string and later calls.split("\n"), which will raise an AttributeError if a tuple is returned. Also, validatest_filethe same way you validate the CSV.Apply this patch to always return a string (or raise) and to validate both inputs:
-def generate_debugger_file(csv_file, st_file): - if not os.path.isfile(csv_file) or not csv_file.lower().endswith(".csv"): - print( - f"Error: Invalid file '{csv_file}'. A path to a csv file is expected.", - file=sys.stderr, - ) - return None, None - - controler = ProjectController() - controler.SetCSVFile(csv_file) - return controler.Generate_embedded_plc_debugger(st_file)[1] +def generate_debugger_file(csv_file, st_file): + if not os.path.isfile(csv_file) or not csv_file.lower().endswith(".csv"): + raise ValueError(f"Invalid file '{csv_file}'. A path to a CSV file is expected.") + if not os.path.isfile(st_file) or not st_file.lower().endswith(".st"): + raise ValueError(f"Invalid file '{st_file}'. A path to an ST file is expected.") + controler = ProjectController() + controler.SetCSVFile(csv_file) + _, debug_text = controler.Generate_embedded_plc_debugger(st_file) + return debug_text
148-151: Derive the ST filename safely; current replace() risks overwriting the XML.Using
.replace("plc.xml", "program.st")will fail for most filenames (e.g.,project.xml) and could overwrite the XML. Useos.path.splitextinstead.- st_file = os.path.abspath(args.generate_st).replace("plc.xml", "program.st") - with open(st_file, "w") as file: - file.write(program_text) + st_file = os.path.splitext(os.path.abspath(args.generate_st))[0] + ".st" + with open(st_file, "w", encoding="utf-8") as file: + file.write(program_text)
🧹 Nitpick comments (3)
xml2st.py (3)
152-153: Nit: Log message no longer matches the action.You’re rewriting ST with complex structs/FBs, not “Parsing complex variables…”. Consider a clearer message.
168-173: Defensive check before appending debugger text.Even with the fixed generator, guard against empty output.
- debug_text = generate_debugger_file( - args.generate_debug[1], args.generate_debug[0] - ) - - append_debugger_to_st(args.generate_debug[0], debug_text) + debug_text = generate_debugger_file(args.generate_debug[1], args.generate_debug[0]) + if not debug_text: + raise RuntimeError("Debugger text generation returned empty output.") + append_debugger_to_st(args.generate_debug[0], debug_text)
157-159: Avoid catching bare Exception; narrow the excepts.Catching all exceptions hides actionable errors and triggers BLE001. Catch the specific ones you raise (e.g., ValueError, OSError) and re-raise unknowns.
Example:
except (ValueError, FileNotFoundError, OSError) as e: print(f"Error generating ST file: {e}", file=sys.stderr) sys.exit(1)Also applies to: 175-176
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
xml2st.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
xml2st.py (1)
ComplexParser.py (3)
ComplexParser(129-528)RewriteST(516-528)AddComplexVars(501-514)
🪛 Ruff (0.12.2)
xml2st.py
157-157: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (2)
xml2st.py (2)
154-156: Correct API usage: RewriteST replaces deprecated ParseSTFile path.Good update aligning with ComplexParser’s public API. This also resolves the earlier review about the removed
ParseSTFilemethod.
163-166: AddComplexVars call looks correct (ST_FILE, CSV_FILE).Matches the new public signature and triggers parsing internally.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
STParser.py (1)
96-103: Array regex end anchor looks goodThis addresses the earlier “missing
$” concern for ARRAY definitions. Nice.ComplexParser.py (4)
165-167: Use a specific parser-state exception instead of bare ExceptionImproves semantics and lint compliance.
- if self.blocks == []: - raise Exception("No block opened to close.") + if self.blocks == []: + raise ParserStateError("No block opened to close.") @@ - raise Exception("No block opened to append line.") + raise ParserStateError("No block opened to append line.")Add near the top (outside range):
class ParserStateError(Exception): passAlso applies to: 186-187
503-507: Raise FileNotFoundError with accurate messages for invalid pathsClearer errors; matches typical Python usage.
- if not st_file or not os.path.isfile(st_file): - raise Exception("ST file not valid. Please provide a valid ST file path.") + if not st_file or not os.path.isfile(st_file): + raise FileNotFoundError("ST file not valid. Provide a valid ST file path.") @@ - if not csv_file or not os.path.isfile(csv_file): - raise Exception("ST file not valid. Please provide a valid CSV file path.") + if not csv_file or not os.path.isfile(csv_file): + raise FileNotFoundError("CSV file not valid. Provide a valid CSV file path.") @@ - if not st_file or not os.path.isfile(st_file): - raise Exception("ST file not valid. Please provide a valid ST file path.") + if not st_file or not os.path.isfile(st_file): + raise FileNotFoundError("ST file not valid. Provide a valid ST file path.")Also applies to: 520-521
380-392: Fix array index expansion to use declared bounds (not zero-based offsets)Current code emits .table[0..N] regardless of lower bound; breaks for ARRAY[3..5], etc.
- for i in range(0, block.end + 1 - block.start): - indexed_prefix = f"{array_prefix}.table[{i}]" + for idx in range(block.start, block.end + 1): + indexed_prefix = f"{array_prefix}.table[{idx}]" if block.data_type in BASE_TYPES: self.csv_vars.append( - {"name": indexed_prefix, "type": block.data_type} + {"name": indexed_prefix, "type": block.data_type} ) else: type = self.__getCustomType(block.data_type) if type: self.__spreadDeclarations( - type, prefix=indexed_prefix, value_added=True + type, prefix=indexed_prefix, value_added=True )
473-479: Guard missing/invalid var number before Ticktime to avoid AttributeErrorHandle non-matching lines safely.
- if ticktime_idx > 0: - extracted_var_number = var_number_pattern.match( - content[ticktime_idx - 1] - ).group(1) - if extracted_var_number: - last_var_number = int(extracted_var_number) + if ticktime_idx > 0: + m = var_number_pattern.match(content[ticktime_idx - 1]) + if m: + last_var_number = int(m.group(1))
🧹 Nitpick comments (6)
STParser.py (2)
121-126: Anchor PROGRAM_DEFINITION regex to EOL to prevent partial matchesAdd the trailing end-of-line anchor.
self.definition = re.compile( r"^\s*PROGRAM\s+" r"(?P<instance>[a-zA-Z_][a-zA-Z0-9_]*)\s+WITH\s+" r"(?P<task>[a-zA-Z_][a-zA-Z0-9_]*)\s*:\s*" - r"(?P<program>[a-zA-Z_][a-zA-Z0-9_]*)\s*;" + r"(?P<program>[a-zA-Z_][a-zA-Z0-9_]*)\s*;\s*$" )
3-24: Use an immutable set for BASE_TYPES for faster membership and safetyConverts repeated membership checks to O(1) and prevents accidental mutation.
-BASE_TYPES = [ +BASE_TYPES = frozenset([ "BOOL", "SINT", "INT", "DINT", "LINT", "USINT", "UINT", "UDINT", "ULINT", "REAL", "LREAL", "TIME", "DATE", "TOD", "DT", "STRING", "BYTE", "WORD", "DWORD", "LWORD" -] +])ComplexParser.py (4)
330-333: Make Jinja2 autoescape intent explicitThese templates aren’t HTML/XML; set autoescape=False to be explicit and quiet linters.
- template = Environment(loader=self.__loader).get_template( + template = Environment(loader=self.__loader, autoescape=False).get_template( FUNCTION_BLOCK_ST_TEMPLATE )
334-336: Rename ambiguous loop variableAvoid single-letter “l”.
- for struct in self.complex_structs: - lines = [l.strip() for l in self.__getBlockLines(struct, False)[1:-1]] + for struct in self.complex_structs: + lines = [line.strip() for line in self.__getBlockLines(struct, False)[1:-1]]
480-480: Make Jinja2 autoescape intent explicit (CSV template as well)Same rationale as above.
- template = Environment(loader=self.__loader).get_template(CSV_VARS_TEMPLATE) + template = Environment(loader=self.__loader, autoescape=False).get_template(CSV_VARS_TEMPLATE)
19-23: Typo: “GLOABL” → “GLOBAL”Cosmetic.
-## GLOABL VARIABLES +## GLOBAL VARIABLES
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
ComplexParser.py(1 hunks)STParser.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-02T21:59:52.612Z
Learnt from: dcoutinho1328
PR: Autonomy-Logic/xml2st#11
File: ComplexParser.py:214-233
Timestamp: 2025-09-02T21:59:52.612Z
Learning: In the ComplexParser.py __getBlockLines method, the ignoreComplexStructs parameter doesn't need to be propagated in recursive calls because complex structs only appear at the first function call level, not nested within other structures in ST files.
Applied to files:
ComplexParser.py
🧬 Code graph analysis (1)
ComplexParser.py (2)
util/paths.py (1)
AbsDir(36-37)STParser.py (5)
GetInfo(33-39)GetInfo(49-56)GetInfo(81-90)GetInfo(105-115)GetInfo(128-137)
🪛 Ruff (0.12.2)
ComplexParser.py
166-166: Create your own exception
(TRY002)
166-166: Avoid specifying long messages outside the exception class
(TRY003)
186-186: Create your own exception
(TRY002)
186-186: Avoid specifying long messages outside the exception class
(TRY003)
330-330: By default, jinja2 sets autoescape to False. Consider using autoescape=True or the select_autoescape function to mitigate XSS vulnerabilities.
(S701)
335-335: Ambiguous variable name: l
(E741)
466-466: Avoid specifying long messages outside the exception class
(TRY003)
480-480: By default, jinja2 sets autoescape to False. Consider using autoescape=True or the select_autoescape function to mitigate XSS vulnerabilities.
(S701)
504-504: Create your own exception
(TRY002)
504-504: Avoid specifying long messages outside the exception class
(TRY003)
507-507: Create your own exception
(TRY002)
507-507: Avoid specifying long messages outside the exception class
(TRY003)
521-521: Create your own exception
(TRY002)
521-521: Avoid specifying long messages outside the exception class
(TRY003)
2 participants
Summary by CodeRabbit
New Features
Changes
Chores