Conversation
Reviewer's GuideThis PR integrates SVG path-based rectangle computation for cluster/group rendering, refines graph input handling by renaming and extending the PyGraphviz loader, introduces path element support in NodeFactory, adjusts image label positioning, and updates package metadata for discoverability and testing. Sequence Diagram for SVG Path Processing in NodeFactorysequenceDiagram
participant NF as NodeFactory.from_svg
participant SVGUtil as SVG Utility
participant RFP as rect_from_svg_path
participant SPP as svg_path.parse_path
participant CT as CoordsTranslate
NF->>SVGUtil: get_first(g, "path")
alt path element found
SVGUtil-->>NF: return path_element
NF->>RFP: rect_from_svg_path(self.coords, path.attrib["d"])
RFP->>SPP: parse_path(path_d)
SPP-->>RFP: parsed_path object
RFP->>RFP: Calculate min/max x, y from parsed_path
RFP->>CT: translate(min_x, min_y)
CT-->>RFP: (translated_x, translated_y)
RFP-->>NF: Rect(x, y, width, height)
NF->>NF: Extract fill, stroke for new shape
end
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. """ WalkthroughThis update introduces new logic for handling SVG path elements in node factories, adds a utility for calculating bounding rectangles from SVG paths, and refines heuristics for loading pygraphviz graphs. It also adjusts style and metadata settings, including image label positioning and package keywords, updates project configuration files, adds a new error class, and increments the version. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant NodeFactory
participant RectFactory
User->>NodeFactory: from_svg(svg_group)
NodeFactory->>NodeFactory: Detects <path> element
NodeFactory->>RectFactory: rect_from_svg_path(coords, path_d)
RectFactory-->>NodeFactory: Bounding Rect
NodeFactory-->>User: Node with computed Rect and style
sequenceDiagram
participant User
participant graphviz2drawio
participant pygraphviz
User->>graphviz2drawio: convert(graph_input)
graphviz2drawio->>graphviz2drawio: _load_pygraphviz_agraph(graph_input)
alt graph_input is str with .dot/.gv/.txt
graphviz2drawio->>pygraphviz: AGraph(filename)
else graph_input is graph string
graphviz2drawio->>pygraphviz: AGraph(string)
else graph_input is other type
graphviz2drawio->>pygraphviz: AGraph(graph_input)
end
graphviz2drawio-->>User: Converted output
Possibly related PRs
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✨ Finishing Touches
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 (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @hbmartin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello! Gemini here, providing a summary of this pull request by @hbmartin. This PR focuses on improving the conversion from Graphviz to Draw.io by adding specific handling for SVG path elements. This is particularly important for correctly rendering Graphviz clusters and groups, which are often represented as paths in the generated SVG output. The changes introduce logic to parse SVG path data (d attribute) to accurately determine the bounding box of these elements, ensuring they are positioned and sized correctly in the resulting Draw.io diagram.
Highlights
- SVG Path Handling: Introduced new functionality to parse SVG
<path>elements and calculate their bounding boxes based on the path data. - Cluster/Group Support: The primary goal of the SVG path handling is to correctly display Graphviz clusters and groups, which rely on path representations in the SVG output.
- Node Factory Integration: Updated the node creation logic to recognize and process SVG path elements, utilizing the new path bounding box calculation.
- Input Loading Refinement: Minor adjustments were made to the function that loads Graphviz inputs to better distinguish between string inputs representing filenames and those representing graph definitions.
Changelog
Click here to see the changelog
- .idea/misc.xml
- Added configuration for Python compatibility inspection component (lines 7-9).
- graphviz2drawio/graphviz2drawio.py
- Renamed
_load_pygraphviz_graphto_load_pygraphviz_agraph(line 17). - Added explicit checks for file extensions (
.dot,.gv,.txt) and graph string endings (},}) when loading graph inputs from strings (lines 58-61). - Added a comment clarifying string input type (line 70).
- Renamed
- graphviz2drawio/mx/NodeFactory.py
- Imported the new
rect_from_svg_pathfunction (line 13). - Added logic in the
from_svgmethod to detect SVG<path>elements, calculate their bounding box usingrect_from_svg_path, and extract fill/stroke styles (lines 72-76).
- Imported the new
- graphviz2drawio/mx/RectFactory.py
- Imported the
svg.pathlibrary (line 1). - Added a new function
rect_from_svg_paththat parses an SVG path data string (dattribute) and calculates the minimum bounding box containing all path segments (lines 7-22).
- Imported the
- graphviz2drawio/mx/Styles.py
- Changed the
verticalLabelPositionfor theIMAGEstyle frombottomtotop(line 84).
- Changed the
- setup.py
- Added "diagrams" and "Lucidchart" to the project keywords (line 44).
- Added "diagrams" to the
tests_requiredependencies (line 48).
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
From dot to SVG,
A path appears, complex weave.
Draw.io now sees.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
|
Generated with ❤️ by ellipsis.dev |
There was a problem hiding this comment.
Code Review
This pull request introduces valuable new features, particularly the support for SVG path-based nodes and enhancements to graph loading. The code is generally well-structured. I've identified a critical issue regarding potential crashes and accuracy in SVG path parsing, and a couple of medium-severity points related to input handling and maintainability that would be good to address. Overall, good progress on extending the tool's capabilities!
Summary of Findings
- Critical: SVG Path Bounding Box Calculation: In
graphviz2drawio/mx/RectFactory.py, therect_from_svg_pathfunction has a critical issue: it can crash with anIndexErrorif an empty or invalid SVG path string is provided. Additionally, its current bounding box calculation method is inaccurate for paths with curves. Suggested using thesvg.path.Path.bbox()method for correctness and robustness. - Medium: Heuristic for DOT String Detection: In
graphviz2drawio/graphviz2drawio.py, the_load_pygraphviz_agraphfunction uses anendswithcheck for '}' or '}\n' to detect DOT strings, which is a heuristic and could potentially misclassify filenames. A more robust method, like prioritizing the existing regex check or try-catch parsing, was suggested for consideration. - Medium: Maintainability (Too Many Returns): In
graphviz2drawio/graphviz2drawio.py, the_load_pygraphviz_agraphfunction is at the Pylint limit for return statements. While clear for now, further expansion might reduce readability. Suggested considering alternative dispatch patterns for long-term maintainability. - IDE Configuration Change: The change in
.idea/misc.xml(adding PythonCompatibilityInspectionAdvertiser) is an IDE-specific configuration update. This is generally a low-impact change, likely for developer convenience. (Not commented due to severity settings) - Function Renaming: In
graphviz2drawio/graphviz2drawio.py,_load_pygraphviz_graphwas renamed to_load_pygraphviz_agraph. This is a good clarification. (Not commented due to severity settings) - NodeFactory Path Handling: In
graphviz2drawio/mx/NodeFactory.py, new logic was added to detect<path>elements and userect_from_svg_path. The shape is set toRECT, which aligns with the PR's goal of rendering paths as rectangular shapes based on their bounding box. (Not commented due to severity settings) - Style Updates: In
graphviz2drawio/mx/Styles.py, theIMAGEstyle'sverticalLabelPositionwas updated. This is a minor visual adjustment. (Not commented due to severity settings) - Setup.py Updates: Changes in
setup.py(keywords and test dependencies) are standard package metadata updates. (Not commented due to severity settings)
Merge Readiness
This pull request introduces significant improvements. However, there's a critical issue in RectFactory.py concerning SVG path parsing that could lead to crashes or incorrect renderings, and a couple of medium-severity issues that would improve robustness and maintainability. I recommend addressing at least the critical issue before merging. I am not authorized to approve pull requests, so please ensure these changes are reviewed and approved by a maintainer.
| parsed_path: svg_path.Path = svg_path.parse_path(path_d) | ||
| start: svg_path.Move = parsed_path.pop(0) | ||
| min_x = start.start.real | ||
| min_y = start.start.imag | ||
| max_x = start.start.real | ||
| max_y = start.start.imag | ||
| for e in parsed_path: | ||
| min_x = min(min_x, e.start.real, e.end.real) | ||
| min_y = min(min_y, e.start.imag, e.end.imag) | ||
| max_x = max(max_x, e.start.real, e.end.real) | ||
| max_y = max(max_y, e.start.imag, e.end.imag) |
There was a problem hiding this comment.
This implementation for calculating the SVG path's bounding box has a couple of potential issues:
- Unhandled Empty/Invalid Path: If
svg_path.parse_path(path_d)(line 8) returns an empty list (e.g., for an invalid or emptypath_dstring),parsed_path.pop(0)on line 9 will raise anIndexError. It's crucial to check ifparsed_path_obj(the result ofparse_path) is empty before this operation to prevent a crash. - Bounding Box Accuracy for Curves: The current loop (lines 14-18) calculates the bounding box using only the
startandendpoints of path segments. This approach can lead to an incorrect (too small) bounding box for paths containing curves like Bezier segments or elliptical arcs, as their control points or the curve itself might extend beyond the AABB of just their start/end points.
Consider using the bbox() method provided by the svg.path.Path object. It typically handles all segment types correctly and would simplify this logic, addressing both points. How about the suggested refinement below?
| parsed_path: svg_path.Path = svg_path.parse_path(path_d) | |
| start: svg_path.Move = parsed_path.pop(0) | |
| min_x = start.start.real | |
| min_y = start.start.imag | |
| max_x = start.start.real | |
| max_y = start.start.imag | |
| for e in parsed_path: | |
| min_x = min(min_x, e.start.real, e.end.real) | |
| min_y = min(min_y, e.start.imag, e.end.imag) | |
| max_x = max(max_x, e.start.real, e.end.real) | |
| max_y = max(max_y, e.start.imag, e.end.imag) | |
| parsed_path_obj: svg_path.Path = svg_path.parse_path(path_d) | |
| if not parsed_path_obj: | |
| # It's important to handle cases where the path string is empty or invalid. | |
| # Raising an error or returning a default (e.g., zero-sized) Rect are options. | |
| # For this suggestion, let's raise a ValueError to make the issue explicit. | |
| raise ValueError(f"Cannot compute bounding box for empty or invalid SVG path: {path_d}") | |
| # Use the bbox() method from the svg.path library for accurate bounding box calculation. | |
| # This method correctly handles all SVG path segment types, including curves (Bezier, arcs). | |
| min_x, max_x, min_y, max_y = parsed_path_obj.bbox() | |
| # The original code initialized min/max from the first 'Move' segment and then iterated. | |
| # bbox() provides these values directly for the entire path. |
| if graph_to_convert.endswith(("}", "}\n")): | ||
| return AGraph(string=graph_to_convert) |
There was a problem hiding this comment.
The check graph_to_convert.endswith(("}", "}\n")) to identify if graph_to_convert is a DOT string seems like a heuristic. While it might work for many common cases, could there be edge cases where a filename (that isn't a DOT file and doesn't have a .dot/.gv/.txt extension) coincidentally ends with '}' or '}\n', leading to it being incorrectly treated as a DOT string? The subsequent regex check (pattern.search) is more robust for identifying DOT string content. Would it be safer to rely more on the regex for string content identification, or perhaps try parsing as a string first and fall back to filename if parsing fails, to avoid potential misclassification?
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
.idea/misc.xml(1 hunks)graphviz2drawio/graphviz2drawio.py(3 hunks)graphviz2drawio/mx/NodeFactory.py(2 hunks)graphviz2drawio/mx/RectFactory.py(1 hunks)graphviz2drawio/mx/Styles.py(1 hunks)setup.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
graphviz2drawio/mx/NodeFactory.py (2)
graphviz2drawio/mx/RectFactory.py (4)
rect_from_ellipse_svg(59-65)rect_from_image(47-56)rect_from_svg_path(7-22)rect_from_svg_points(25-44)graphviz2drawio/models/SVG.py (1)
get_first(10-11)
🪛 Ruff (0.11.9)
setup.py
44-44: Line too long (104 > 88)
(E501)
🔇 Additional comments (9)
setup.py (2)
48-48: Added 'diagrams' to test dependencies.Adding "diagrams" as a test dependency is appropriate since you're enhancing diagram-related functionality, particularly for clusters and groups. This ensures proper testing of the new features.
44-44:Details
✅ Verification successful
Expanded keywords to better reflect package capabilities.
The keyword list was expanded to include "diagrams" and "Lucidchart", which better represents the package's capabilities, especially with the new path handling features being added.
🏁 Script executed:
#!/bin/bash # Check if the keywords list exceeds PyPI's recommended length keyword_length=$(echo "graphviz graph agraph dot convert conversion diagrams drawio mxgraph maxgraph Lucidchart" | wc -c) echo "Keyword list length: $keyword_length characters" # PyPI typically recommends keeping metadata fields concise if [ $keyword_length -gt 100 ]; then echo "WARNING: Keyword list is quite long ($keyword_length chars). Consider shortening for better indexing." fiLength of output: 227
✅ Keywords list length is acceptable
The updated keywords string totals 89 characters, which is under PyPI’s informal 100-character guidance. The added terms “diagrams” and “Lucidchart” can remain.🧰 Tools
🪛 Ruff (0.11.9)
44-44: Line too long (104 > 88)
(E501)
graphviz2drawio/mx/RectFactory.py (1)
1-2: Added SVG path handling capability.Good addition of the svg.path library import, which enables the new path handling functionality.
graphviz2drawio/mx/NodeFactory.py (1)
10-15: Updated imports to include the new SVG path handling function.The import statement is correctly updated to include the new
rect_from_svg_pathfunction along with the existing imported functions.graphviz2drawio/mx/Styles.py (1)
84-84: Improved image label positioning.Changing the vertical label position from "bottom" to "top" for images provides better visual alignment, particularly when working with clustered diagrams. This complements the other path handling improvements in this PR.
graphviz2drawio/graphviz2drawio.py (4)
17-17: Function call updated to match renamed function.The function call was properly updated to use the new name
_load_pygraphviz_agraphinstead of the previous_load_pygraphviz_graph, maintaining consistency with the function definition below.
52-52: Improved function naming for clarity.The function name change from
_load_pygraphviz_graphto_load_pygraphviz_agraphis more specific and better reflects that it returns an AGraph object. Thenoqa: PLR0911comment appropriately suppresses the linter warning about too many return statements, which is justified given the function's multiple code paths for different input types.
58-61: Enhanced string input handling with explicit heuristics.The addition of specific handling for common file extensions and graph string patterns improves the robustness of the function. This change provides more predictable behavior when working with:
- Files with common Graphviz extensions (.dot, .gv, .txt)
- Graph strings that end with closing braces
This enhancement aligns with the PR objective of improving path handling for better display of clusters and groups.
70-70: Improved code clarity with descriptive comment.The added comment clearly explains what the condition is checking for - that the input string contains a graph/dot definition. This improves code readability and makes the function's logic easier to understand for future maintainers.
| <component name="PythonCompatibilityInspectionAdvertiser"> | ||
| <option name="version" value="3" /> | ||
| </component> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Review IDE configuration inclusion
The new <component name="PythonCompatibilityInspectionAdvertiser"> entry enables Python 3 compatibility inspections in JetBrains IDEs.
Please confirm that this IDE-specific setting should be checked into version control. If it’s not required for all contributors, consider removing it or excluding .idea/misc.xml via .gitignore to keep the repository clean.
🤖 Prompt for AI Agents
In the .idea/misc.xml file at lines 7 to 9, the addition of the
PythonCompatibilityInspectionAdvertiser component enables Python 3 compatibility
inspections in JetBrains IDEs. Verify if this IDE-specific configuration is
necessary for all contributors; if not, remove this component from the file or
exclude .idea/misc.xml from version control by adding it to .gitignore to avoid
cluttering the repository with personal IDE settings.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
graphviz2drawio/mx/RectFactory.py (1)
18-24: 🧹 Nitpick (assertive)Consider using svg.path's bbox() method for improved accuracy.
The current implementation only considers start and end points of path segments, which may lead to inaccurate bounding boxes for curved segments as noted in your comment. The svg.path library provides a more accurate bbox() method.
- # Note that this loop may not be accurate since it does not calculate - # the Bezier curve based on the control points - for e in parsed_path: - min_x = min(min_x, e.start.real, e.end.real) - min_y = min(min_y, e.start.imag, e.end.imag) - max_x = max(max_x, e.start.real, e.end.real) - max_y = max(max_y, e.start.imag, e.end.imag) + # Re-create the path with the start point + full_path = svg_path.Path([start] + list(parsed_path)) + # Use the built-in bbox method for more accurate bounds calculation + min_x, max_x, min_y, max_y = full_path.bbox()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
graphviz2drawio/models/Errors.py(1 hunks)graphviz2drawio/mx/RectFactory.py(1 hunks)graphviz2drawio/version.py(1 hunks)specs/directed/subgraph.xml(0 hunks)test/directed/subgraph.gv.txt(0 hunks)
💤 Files with no reviewable changes (2)
- specs/directed/subgraph.xml
- test/directed/subgraph.gv.txt
🧰 Additional context used
🪛 GitHub Actions: Lint and Test
graphviz2drawio/mx/RectFactory.py
[error] 11-11: mypy type check error: Too few arguments for "CouldNotParsePathError" [call-arg]
🔇 Additional comments (3)
graphviz2drawio/mx/RectFactory.py (2)
1-2: Good addition of the svg.path dependency.The import is correctly added to support the new SVG path handling functionality.
4-4: Appropriate import of the new error class.The CouldNotParsePathError is properly imported from the models.Errors module.
graphviz2drawio/version.py (1)
1-1: Appropriate version bump for feature additions.Incrementing the minor version number from 1.0.0 to 1.1.0 follows semantic versioning conventions for adding new functionality in a backward-compatible manner.
| def rect_from_svg_path(coords: CoordsTranslate, path_d: str) -> Rect: | ||
| parsed_path: svg_path.Path = svg_path.parse_path(path_d) | ||
| if len(parsed_path) == 0 or not isinstance(parsed_path[0], svg_path.Move): | ||
| raise CouldNotParsePathError | ||
| start: svg_path.Move = parsed_path.pop(0) | ||
| min_x = start.start.real | ||
| min_y = start.start.imag | ||
| max_x = start.start.real | ||
| max_y = start.start.imag | ||
|
|
||
| # Note that this loop may not be accurate since it does not calculate | ||
| # the Bezier curve based on the control points | ||
| for e in parsed_path: | ||
| min_x = min(min_x, e.start.real, e.end.real) | ||
| min_y = min(min_y, e.start.imag, e.end.imag) | ||
| max_x = max(max_x, e.start.real, e.end.real) | ||
| max_y = max(max_y, e.start.imag, e.end.imag) | ||
|
|
||
| (x, y) = coords.translate(min_x, min_y) | ||
|
|
||
| return Rect(x=x, y=y, width=max_x - min_x, height=max_y - min_y) |
There was a problem hiding this comment.
Fix the error handling and consider adding a docstring.
The function implementation has two issues:
- When raising CouldNotParsePathError, you're not providing the required Element parameter (causing the mypy error)
- There's no docstring explaining the function's purpose and parameters
#!/bin/bash
# Check the CouldNotParsePathError definition
grep -A 10 "class CouldNotParsePathError" ../graphviz2drawio/models/Errors.pyFor the first issue, either:
- Modify the CouldNotParsePathError to accept an optional Element parameter, or
- Create an Element object to pass to the error
- if len(parsed_path) == 0 or not isinstance(parsed_path[0], svg_path.Move):
- raise CouldNotParsePathError
+ if len(parsed_path) == 0 or not isinstance(parsed_path[0], svg_path.Move):
+ # Use a more appropriate error that doesn't require an Element
+ raise GdValueError(f"Could not parse path: {path_d}")Additionally, add a docstring:
def rect_from_svg_path(coords: CoordsTranslate, path_d: str) -> Rect:
+ """Calculate a bounding rectangle for an SVG path.
+
+ Args:
+ coords: Coordinate translator for adjusting positions
+ path_d: SVG path data string (d attribute)
+
+ Returns:
+ Rect: A rectangle encompassing the path
+
+ Raises:
+ GdValueError: If the path cannot be parsed or does not start with a Move command
+ """
parsed_path: svg_path.Path = svg_path.parse_path(path_d)🧰 Tools
🪛 GitHub Actions: Lint and Test
[error] 11-11: mypy type check error: Too few arguments for "CouldNotParsePathError" [call-arg]
🤖 Prompt for AI Agents
In graphviz2drawio/mx/RectFactory.py between lines 8 and 28, fix the error
handling by passing the required Element parameter when raising
CouldNotParsePathError, either by creating an appropriate Element instance or
modifying the error class to accept an optional parameter. Also, add a clear
docstring at the start of the rect_from_svg_path function describing its
purpose, parameters, and return value.
| class CouldNotParsePathError(GdValueError): | ||
| """Could not parse path from SVG element.""" | ||
|
|
||
| def __init__(self, g: Element) -> None: | ||
| super().__init__( | ||
| f"Could not parse path from SVG element: {g.tag} ({g.attrib})", | ||
| ) | ||
|
|
There was a problem hiding this comment.
Well-structured error class, but fix the inconsistency with its usage.
The new error class follows the pattern of other error classes in the file. However, there's an inconsistency with how it's used in RectFactory.py, where it's raised without the required Element parameter.
Either:
- Modify this class to accept an optional Element parameter, or
- Update the calling code in RectFactory.py to provide an Element
class CouldNotParsePathError(GdValueError):
"""Could not parse path from SVG element."""
- def __init__(self, g: Element) -> None:
+ def __init__(self, g: Element = None) -> None:
super().__init__(
- f"Could not parse path from SVG element: {g.tag} ({g.attrib})",
+ f"Could not parse path from SVG element: {g.tag if g else 'unknown'} ({g.attrib if g else 'no attributes'})",
)Alternatively, consider creating a more general version of this error for cases where an Element isn't available.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class CouldNotParsePathError(GdValueError): | |
| """Could not parse path from SVG element.""" | |
| def __init__(self, g: Element) -> None: | |
| super().__init__( | |
| f"Could not parse path from SVG element: {g.tag} ({g.attrib})", | |
| ) | |
| class CouldNotParsePathError(GdValueError): | |
| """Could not parse path from SVG element.""" | |
| def __init__(self, g: Element = None) -> None: | |
| super().__init__( | |
| f"Could not parse path from SVG element: {g.tag if g else 'unknown'} ({g.attrib if g else 'no attributes'})", | |
| ) |
🤖 Prompt for AI Agents
In graphviz2drawio/models/Errors.py around lines 13 to 20, the
CouldNotParsePathError class requires an Element parameter in its constructor,
but it is raised without this parameter in RectFactory.py, causing
inconsistency. To fix this, either modify the CouldNotParsePathError class to
accept an optional Element parameter and handle the case when it is None, or
update the RectFactory.py code to always provide the Element when raising this
error. Alternatively, create a more general error class for cases without an
Element and use that where appropriate.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
graphviz2drawio/mx/RectFactory.py (1)
8-28: 🧹 Nitpick (assertive)Add a docstring to improve code documentation.
The new function works well for calculating bounding rectangles from SVG paths, but lacks a docstring explaining its purpose, parameters, and return value.
def rect_from_svg_path(coords: CoordsTranslate, path_d: str) -> Rect: + """Calculate a bounding rectangle for an SVG path. + + Args: + coords: Coordinate translator for adjusting positions + path_d: SVG path data string (d attribute) + + Returns: + Rect: A rectangle encompassing the path + + Raises: + CouldNotParsePathError: If the path cannot be parsed or does not start with a Move command + """ parsed_path: svg_path.Path = svg_path.parse_path(path_d)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
graphviz2drawio/models/Errors.py(1 hunks)graphviz2drawio/mx/RectFactory.py(1 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
graphviz2drawio/mx/RectFactory.py (3)
graphviz2drawio/models/CoordsTranslate.py (2)
CoordsTranslate(1-15)translate(9-10)graphviz2drawio/models/Errors.py (1)
CouldNotParsePathError(13-19)graphviz2drawio/models/Rect.py (1)
Rect(1-62)
🔇 Additional comments (4)
pyproject.toml (1)
13-13: LGTM: Added "RSE102" to Ruff's ignore list.The addition of "RSE102" to the lint.ignore list aligns with the introduction of the new
CouldNotParsePathErrorclass in this PR.graphviz2drawio/models/Errors.py (1)
13-20: Good implementation of the new error class.The
CouldNotParsePathErrorfollows the pattern of other error classes in the file and properly inherits fromGdValueError. The constructor now accepts a path string parameter instead of an Element, which matches how it's used in RectFactory.py.graphviz2drawio/mx/RectFactory.py (2)
1-5: Correct imports for the new functionality.The imports for svg.path and CouldNotParsePathError are appropriate for the new SVG path handling functionality.
10-11: Error handling looks good.The function correctly raises CouldNotParsePathError with the path data when the path is empty or doesn't start with a Move command, which matches the error class definition.
| # Note that this loop may not be accurate since it does not calculate | ||
| # the Bezier curve based on the control points | ||
| for e in parsed_path: | ||
| min_x = min(min_x, e.start.real, e.end.real) | ||
| min_y = min(min_y, e.start.imag, e.end.imag) | ||
| max_x = max(max_x, e.start.real, e.end.real) | ||
| max_y = max(max_y, e.start.imag, e.end.imag) | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider using the svg.path library's bbox() method for more accurate results.
The current implementation calculates the bounding box using only the start and end points of path segments, which may not accurately capture the full extent of Bezier curves. The svg.path library provides a bbox() method that can handle curve control points more accurately.
- # Note that this loop may not be accurate since it does not calculate
- # the Bezier curve based on the control points
- for e in parsed_path:
- min_x = min(min_x, e.start.real, e.end.real)
- min_y = min(min_y, e.start.imag, e.end.imag)
- max_x = max(max_x, e.start.real, e.end.real)
- max_y = max(max_y, e.start.imag, e.end.imag)
+ # Use the bbox() method for more accurate results with curves
+ # Create a new Path with the start element added back
+ full_path = svg_path.Path(start)
+ full_path.extend(parsed_path)
+ min_x, max_x, min_y, max_y = full_path.bbox()🌐 Web query:
Does the svg.path library in Python provide a bbox() method for calculating bounding boxes of paths?
💡 Result:
The svg.path library in Python does not provide a built-in bbox() method for calculating the bounding box of paths or path segments. According to the official documentation and example usages, the library offers fundamental path segment classes (like Line, Arc, CubicBezier, QuadraticBezier) and a Path container, along with methods for parsing paths, evaluating points, and computing lengths[3]. However, there is no mention of a bounding box (bbox) method within its API or usage examples.
By contrast, libraries such as svgpathtools (which is sometimes referenced for similar tasks) do implement bbox() methods on individual path segments, and the community often uses it for bounding box calculations[1][5][7]. If you require bounding box functionality specifically in Python, you might consider switching to or supplementing with svgpathtools or svgelements, both of which provide more advanced geometric utilities—including bounding box calculations[1][6].
In summary:
svg.pathdoes not provide abbox()method out of the box for paths or segments[3].- For bounding box calculations in Python, use alternative libraries like
svgpathtoolsorsvgelements[1][6].
Citations:
- 1: https://pypi.org/project/svgpathtools/
- 2: https://github.com/mondeja/svg-path-bbox
- 3: https://pypi.org/project/svg.path/
- 4: https://www.npmjs.com/package/svg-path-bbox/v/0.0.16
- 5: https://github.com/mathandy/svgpathtools/blob/master/svgpathtools/path.py
- 6: https://pypi.org/project/svgelements/
- 7: https://www.youtube.com/watch?v=atAHcsycLpA
- 8: https://www.w3.org/TR/SVG/paths.html
- 9: https://app.soos.io/research/packages/Python/-/svgpathtools/
- 10: Calculate bounding box Kozea/CairoSVG#348
Switch to a library with built-in bbox() or implement a manual bbox calculation
The svg.path library does not include a bbox() method, so the original suggestion to call full_path.bbox() on an svg.path.Path will not work. For accurate bounding boxes of Bezier curves you have two main options:
• Use svgpathtools (or svgelements) which provide a bbox() 方法 out of the box.
• Keep svg.path but compute extrema of each segment’s control points yourself.
File needing update:
graphviz2drawio/mx/RectFactory.py(around lines 18–25)
Suggested diff using svgpathtools:
-import svg.path as svg_path
+from svgpathtools import Path as SVGPath
# Calculate bounding box including all control points
- for e in parsed_path:
- min_x = min(min_x, e.start.real, e.end.real)
- min_y = min(min_y, e.start.imag, e.end.imag)
- max_x = max(max_x, e.start.real, e.end.real)
- max_y = max(max_y, e.start.imag, e.end.imag)
+ # parsed_path must be an svgpathtools.Path, not svg.path.Path
+ pt_path = SVGPath(*parsed_path)
+ min_x, max_x, min_y, max_y = pt_path.bbox()If you prefer not to add a dependency, you’ll need to find the extremal points of each segment’s polynomial equations manually and factor in their control points.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In graphviz2drawio/mx/RectFactory.py around lines 18 to 25, the current bounding
box calculation only considers start and end points of path segments, which is
inaccurate for Bezier curves. Since svg.path lacks a bbox() method, fix this by
either switching to the svgpathtools or svgelements library that provide bbox()
methods for accurate bounding box calculation, or implement manual extrema
calculations for each segment's control points to determine the true bounding
box. Update the code to use one of these approaches to ensure the bounding box
fully encompasses the curves.
|
1 participant
Summary by Sourcery
Add SVG path handling for accurate cluster/group rendering, improve DOT graph loading logic, adjust default image label positioning, and update package metadata.
New Features:
Enhancements:
_load_pygraphviz_agraphChores:
svg.pathanddiagramsto dependencies and update package keywords to include LucidchartSummary by CodeRabbit
<path>elements.diagramslibrary and verifying conversion output.