Rtop 35 add support for complex variables on debug.c #8
Conversation
|
Warning Rate limit exceeded@dcoutinho1328 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 18 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant xml2st.py
participant ComplexParser
participant FileSystem
participant Jinja2
User->>xml2st.py: Run with --generate-st
xml2st.py->>FileSystem: Save generated ST file
xml2st.py->>ComplexParser: parse_complex_variables(st_file)
ComplexParser->>FileSystem: Read ST file
ComplexParser->>Jinja2: Render code blocks for complex variables
ComplexParser->>FileSystem: Write processed ST file
ComplexParser-->>xml2st.py: Return processed content
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ComplexParser.py (1)
76-90: Consider more robust parsing for variable declarations.The current parsing logic assumes specific formatting with colons and semicolons. This might break if the ST file has different formatting (e.g., extra spaces, different line breaks).
Consider using a more robust parsing approach, such as:
- Regular expressions to handle varying whitespace
- A proper ST parser library if available
- More comprehensive error handling for malformed lines
Example improvement:
import re # Pattern for variable declaration with optional initialization var_pattern = re.compile(r'^\s*(\w+)\s*:\s*(\w+)(?:\s*:=\s*([^;]+))?\s*;?\s*$') match = var_pattern.match(line) if match: name, type_name, value = match.groups() complex_vars[-1][1][name] = {"type": type_name, "value": value}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.devcontainer/devcontainer.json(1 hunks)ComplexParser.py(1 hunks)ProjectController.py(3 hunks)templates/function_block.st.j2(1 hunks)xml2st.py(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
ProjectController.py (1)
util/paths.py (1)
AbsDir(36-37)
ComplexParser.py (1)
util/paths.py (1)
AbsDir(36-37)
🪛 Ruff (0.12.2)
ComplexParser.py
53-53: Ambiguous variable name: l
(E741)
🔇 Additional comments (6)
.devcontainer/devcontainer.json (1)
20-21: LGTM!The addition of the Structured Text VS Code extension is appropriate for the new ST file parsing capabilities introduced in this PR.
ProjectController.py (2)
10-12: Good refactoring!Centralizing the template loader initialization avoids redundant instantiation and improves performance.
164-164: LGTM!Correctly reuses the pre-initialized loader instance.
xml2st.py (2)
60-72: LGTM!The function properly validates the input file and handles errors appropriately.
123-125: Should complex variable parsing also apply to debug mode?Complex variable parsing is performed after generating the ST file with
--generate-st, but not with--generate-debug. Is this intentional, or should complex variables also be parsed when debug information is included?templates/function_block.st.j2 (1)
11-15: Is the hardcodedlocal_tempvariable intentional?The template always declares and initializes a
local_tempvariable to 0. Is this a placeholder that should be parameterized, or is it intentionally hardcoded for a specific purpose?
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
ComplexParser.py (2)
38-39: Fix incomplete exception message.The exception message is incomplete and ends with "Please ". This needs to be completed with specific instructions.
53-54: Rename ambiguous variable name.The variable
lis ambiguous and reduces code readability. This issue was previously identified and should be addressed.Also applies to: 99-100, 102-102
🧹 Nitpick comments (2)
xml2st.py (2)
61-76: Well-structured function with good validation and error handling.The function properly validates the ST file format and handles parsing exceptions appropriately. Consider whether the inconsistent behavior (returning None vs sys.exit) between file validation and parsing errors is intentional.
42-42: Remove unnecessary f-string prefix.The f-string has no placeholders, so the
fprefix is unnecessary and should be removed.- print(f"Error: Failed to load XML project file.", file=sys.stderr) + print("Error: Failed to load XML project file.", file=sys.stderr)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ComplexParser.py(1 hunks)xml2st.py(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
ComplexParser.py (1)
util/paths.py (1)
AbsDir(36-37)
xml2st.py (2)
ComplexParser.py (2)
ComplexParser(9-104)ParseSTFile(33-104)plcopen/plcopen.py (1)
LoadProject(325-329)
🪛 Ruff (0.12.2)
ComplexParser.py
53-53: Ambiguous variable name: l
(E741)
xml2st.py
42-42: f-string without any placeholders
Remove extraneous f prefix
(F541)
⏰ 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 (6)
ComplexParser.py (3)
11-14: Constructor implementation looks good.The Jinja2 FileSystemLoader initialization is properly configured using the absolute directory path.
16-31: Template-based code generation implementation is solid.The method correctly uses Jinja2 templating to generate function blocks and saves the processed output with an appropriate naming convention.
56-91: Parsing logic is well-designed and handles edge cases properly.The state machine correctly tracks TYPE/STRUCT boundaries, prevents invalid nesting, and properly extracts variable information including optional initial values.
xml2st.py (3)
8-8: Import statement is properly placed.The ComplexParser import follows the existing import structure and is correctly positioned.
14-16: Excellent improvement in error output handling.Redirecting error messages to
sys.stderrfollows best practices and improves the separation of normal output from error output.Also applies to: 30-30, 36-36, 42-42
126-132: Clean integration of complex variable parsing into the workflow.The sequential flow from ST generation to file saving to complex variable parsing is logical and well-structured.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
ComplexParser.py (2)
38-39: Fix incomplete exception message.The exception message is incomplete and ends with "Please ".
53-54: Rename ambiguous variable name.Using
las a variable name is ambiguous and reduces readability. Also update references on lines 85 and 88.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ComplexParser.py(1 hunks)templates/function_block.st.j2(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- templates/function_block.st.j2
🧰 Additional context used
🧬 Code Graph Analysis (1)
ComplexParser.py (1)
util/paths.py (1)
AbsDir(36-37)
🪛 Ruff (0.12.2)
ComplexParser.py
53-53: 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 (3)
ComplexParser.py (3)
1-7: LGTM!The imports are appropriate and the constants are well-defined to avoid magic strings throughout the code.
9-14: LGTM!The initialization correctly sets up the Jinja2 FileSystemLoader with an absolute path, ensuring templates are found regardless of the current working directory.
16-31: LGTM!The method correctly uses Jinja2 templating to generate code blocks for complex variables and properly handles file operations with context managers.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ComplexParser.py (1)
53-54: Fix ambiguous variable name as flagged in previous review.The variable name
lis still present and flagged by static analysis. This was identified in a previous review and should be addressed.Apply this diff to fix the ambiguous variable name:
- for l in lines: - line = l.strip() + for line_raw in lines: + line = line_raw.strip()Also update the reference on line 88:
- new_lines.append(l) + new_lines.append(line_raw)
🧹 Nitpick comments (1)
ComplexParser.py (1)
33-90: Consider adding input validation and error handling.The parsing method could benefit from additional robustness:
- File existence check: Validate that the file exists before attempting to read it
- File reading error handling: Wrap file operations in try-catch blocks
- Template rendering error handling: Handle potential Jinja2 template errors
def ParseSTFile(self, file): """ Parse ST file to extract complex variables. """ if not file: raise Exception("ST file not valid. Please provide a valid ST file path.") + + if not os.path.exists(file): + raise Exception(f"ST file does not exist: {file}") self.__stFile = file lines = [] - with open(self.__stFile, "r") as f: - lines = f.readlines() + try: + with open(self.__stFile, "r") as f: + lines = f.readlines() + except IOError as e: + raise Exception(f"Error reading ST file: {e}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ComplexParser.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
ComplexParser.py (1)
util/paths.py (1)
AbsDir(36-37)
🪛 Ruff (0.12.2)
ComplexParser.py
53-53: 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 (7)
ComplexParser.py (7)
1-7: LGTM! Clean imports and well-defined constants.The imports are appropriate for the functionality, and the constants follow good practices by avoiding magic strings throughout the code.
9-14: LGTM! Proper template loader initialization.The constructor correctly sets up the Jinja2 FileSystemLoader using the utility function to resolve the absolute path to the templates directory.
16-31: LGTM! Well-structured template rendering and file output.The method correctly:
- Renders Jinja2 templates for each complex variable
- Appends remaining ST file content
- Writes to a properly named output file
- Returns the generated content for flexibility
The implementation is clean and follows good practices.
38-39: Exception message has been properly fixed.The incomplete exception message from the previous review has been addressed with a clear and informative error message.
58-71: LGTM! Proper validation and parsing logic for STRUCT declarations.The code correctly:
- Validates that STRUCT declarations are not nested
- Ensures STRUCTs appear within TYPE blocks
- Extracts the variable name and initializes the complex variable structure
The validation logic prevents malformed ST files from being processed incorrectly.
79-83: Bounds checking has been properly implemented.The previous review concern about accessing
new_lines[-1]without bounds checking has been addressed. The code now properly checksif new_lines and TYPE_TOKEN in new_lines[-1]:before accessing the last element.
84-86: LGTM! Smart empty line handling.The logic to skip consecutive empty lines is well-implemented and will help produce cleaner output by avoiding unnecessary blank lines.
thiagoralves
deleted the
RTOP-35-Add-support-for-complex-variables-on-debug.c
branch
2 participants
Summary by CodeRabbit