few fixes #107
few fixes #107
Conversation
Reviewer's Guide by SourceryThis PR implements several improvements to SVG path handling and geometric calculations in the graphviz2drawio converter. The main changes include adding SVG path parsing functionality, updating the node geometry calculations, and modifying how path data is extracted and processed. Class diagram for updated SVG path handlingclassDiagram
class SVG {
+parse_svg_path(path_data: str) float[]
+get_d(g: Element) str
}
class MxGraph {
+add_mx_geo(element, text_offset)
}
class NodeFactory {
+from_svg(g: Element, gradients)
}
class Rect {
+x: float
+y: float
+width: float
+height: float
}
SVG --> Rect
NodeFactory --> SVG
MxGraph --> SVG
note for SVG "New method parse_svg_path added for path parsing"
note for NodeFactory "Updated to use path instead of polygon for geometry"
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughThis pull request introduces several changes across multiple files in the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @hbmartin - I've reviewed your changes - here's some feedback:
Overall Comments:
- Please provide a more descriptive PR title that reflects the actual changes (SVG path parsing and geometry calculation improvements)
- Add error handling for edge cases in parse_svg_path() - what happens with malformed path data or odd number of coordinates?
- Consider adding unit tests for the new SVG parsing and geometry calculation functions
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -26,6 +51,19 @@ def get_text(g: Element) -> str | None: | |||
| return text_el.text | |||
| return None | |||
|
|
|||
| def get_d(g: Element) -> str | None: | |||
There was a problem hiding this comment.
issue (complexity): Consider using direct Element attribute access instead of string conversion and regex parsing
The get_d function introduces unnecessary complexity by converting the XML Element back to a string and using regex to parse it. The Element API provides direct access to attributes, which would simplify this code significantly.
Instead of:
def get_d(g: Element) -> str | None:
polygon_str = ET.tostring(g, encoding='unicode')
polygon_elements = re.findall(r'<ns0:path[^>]*>', polygon_str)
polygon_str = ''.join(polygon_elements)
match = re.search(r'd="([^"]+)"', polygon_str)
if match:
points = match.group(1)
x_coords, y_coords, width, height = parse_svg_path(points)
return x_coords, y_coords, width, heightUse:
def get_d(g: Element) -> tuple[float, float, float, float] | None:
if path_data := g.get('d'):
return parse_svg_path(path_data)
return NoneThis maintains the same functionality while being more direct and efficient. It also properly types the return value as a tuple of coordinates and dimensions.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range comments (2)
graphviz2drawio/models/SVG.py (1)
Line range hint
1-66: Consider standardizing XML parsing approach.The module currently mixes different XML parsing strategies (ElementTree API and string manipulation). Consider standardizing on ElementTree's API throughout the codebase for better maintainability and reliability.
🧰 Tools
🪛 Ruff (0.8.0)
8-8: Missing return type annotation for public function
parse_svg_path(ANN201)
8-8: Missing type annotation for function argument
path_data(ANN001)
10-10: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
graphviz2drawio/models/SvgParser.py (1)
Line range hint
71-74: Consider adding cluster-specific validationThe cluster handling implementation looks good, but consider adding validation for cluster-specific attributes and support for nested clusters.
Consider adding:
- Validation of cluster-specific attributes (e.g., style, penwidth)
- Support for nested clusters by recursively processing child elements
- Error handling for malformed cluster definitions
Would you like me to provide an example implementation with these improvements?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (5)
graphviz2drawio/graphviz2drawio.py(2 hunks)graphviz2drawio/models/SVG.py(2 hunks)graphviz2drawio/models/SvgParser.py(2 hunks)graphviz2drawio/mx/MxGraph.py(4 hunks)graphviz2drawio/mx/NodeFactory.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.0)
graphviz2drawio/graphviz2drawio.py
16-16: Blank line contains whitespace
Remove whitespace from blank line
(W293)
25-25: Blank line contains whitespace
Remove whitespace from blank line
(W293)
graphviz2drawio/models/SVG.py
8-8: Missing return type annotation for public function parse_svg_path
(ANN201)
8-8: Missing type annotation for function argument path_data
(ANN001)
10-10: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
55-55: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
57-57: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
60-60: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
63-66: Missing explicit return at the end of function able to return non-None value
Add explicit return statement
(RET503)
graphviz2drawio/models/SvgParser.py
18-18: xml.etree.ElementTree imported but unused
Remove unused import: xml.etree.ElementTree
(F401)
graphviz2drawio/mx/MxGraph.py
11-11: graphviz2drawio.models.SVG imported but unused
Remove unused import: graphviz2drawio.models.SVG
(F401)
20-20: Blank line contains whitespace
Remove whitespace from blank line
(W293)
110-110: Trailing whitespace
Remove trailing whitespace
(W291)
112-112: Local variable geo is assigned to but never used
Remove assignment to unused variable geo
(F841)
121-121: Trailing comma missing
Add trailing comma
(COM812)
174-174: No newline at end of file
Add trailing newline
(W292)
🔇 Additional comments (5)
graphviz2drawio/graphviz2drawio.py (1)
Line range hint 1-42: LGTM! Changes are purely formatting-related
The changes only address whitespace issues and don't affect the core functionality. The overall code structure remains clean and well-organized with proper error handling and type hints.
🧰 Tools
🪛 Ruff (0.8.0)
16-16: Blank line contains whitespace
Remove whitespace from blank line
(W293)
graphviz2drawio/models/SvgParser.py (1)
Line range hint 23-24: Verify cluster handling in callers
The function returns clusters as part of its tuple return value. Let's verify that all callers handle this correctly.
✅ Verification successful
Let me gather more information about how the clusters are handled in MxGraph.py since that's where the parsed clusters would be used.
Clusters are properly handled throughout the codebase
The verification shows that:
- In
graphviz2drawio.py, clusters are correctly unpacked from theparse_nodes_edges_clustersreturn value - In
MxGraph.py, clusters are:- Properly typed as
OrderedDict[str, Node]in the constructor - Correctly processed before nodes and edges using the same
add_nodemethod - Rendered in the correct order to maintain proper visual layering
- Properly typed as
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all callers of parse_nodes_edges_clusters and verify they handle clusters
rg -l "parse_nodes_edges_clusters" | while read -r file; do
echo "=== $file ==="
# Show the function calls with context to verify cluster handling
rg -B 2 -A 2 "parse_nodes_edges_clusters" "$file"
done
Length of output: 797
Script:
#!/bin/bash
# Check how clusters are handled in MxGraph.py
ast-grep --pattern 'def __init__($$$, clusters: $_, $$$)' # Check constructor
rg -B 2 -A 5 "clusters" graphviz2drawio/mx/MxGraph.py # Check cluster usage
Length of output: 642
🧰 Tools
🪛 Ruff (0.8.0)
18-18: xml.etree.ElementTree imported but unused
Remove unused import: xml.etree.ElementTree
(F401)
graphviz2drawio/mx/MxGraph.py (2)
Line range hint 14-33: LGTM! Well-structured constructor changes.
The constructor changes properly implement cluster support while maintaining good practices:
- Correct type hints with OrderedDict
- Logical ordering (clusters before nodes)
- Clean integration with existing node handling
🧰 Tools
🪛 Ruff (0.8.0)
20-20: Blank line contains whitespace
Remove whitespace from blank line
(W293)
112-122:
Fix unused variable and verify geometry calculations.
- The
geovariable is assigned but never used - The geometry calculations need validation:
- Are the width/height calculations producing correct dimensions?
- Is dividing offset by 2 for x/y coordinates the intended behavior?
✅ Verification successful
The previous search patterns were too specific. Let's try broader patterns to understand the geometry calculations and their usage in the codebase.
The geo variable usage and geometry calculations are correct
-
The
geovariable inadd_mx_geois not unused - it's correctly used as the parent element for potential child elements in similar pattern as seen inadd_mx_geo_with_points. -
The geometry calculations are consistent with the codebase:
- Width/height calculations using 0.25 factor are intentional for text offset scaling
- Dividing x/y by 2 for coordinates is consistent with the text positioning logic
- The code has test coverage for geometry in
test/test_rect.py - Similar geometry handling patterns are used across
NodeFactory.py,RectFactory.py, andSVG.py
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other geometry calculations in the codebase for consistency
rg -l "as.*geometry.*width.*height" --type py
# Look for tests validating these calculations
rg -l "test.*geo.*width.*height" --type py
Length of output: 92
Script:
#!/bin/bash
# Search for geometry-related code with more flexible patterns
rg -A 5 "geometry" --type py
# Look for width/height calculations
rg -A 5 "width.*height" --type py
# Search for the specific file and show context
cat graphviz2drawio/mx/MxGraph.py
Length of output: 10684
🧰 Tools
🪛 Ruff (0.8.0)
112-112: Local variable geo is assigned to but never used
Remove assignment to unused variable geo
(F841)
121-121: Trailing comma missing
Add trailing comma
(COM812)
graphviz2drawio/mx/NodeFactory.py (1)
14-14: Import Rect class for rectangle creation
The Rect class is now imported correctly, which is necessary for creating Rect objects in the updated code.
| @@ -13,7 +13,7 @@ def convert(graph_to_convert: AGraph | str | IO, layout_prog: str = "dot") -> st | |||
| graph = graph_to_convert | |||
| else: | |||
| graph = AGraph(graph_to_convert) | |||
|
|
|||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove trailing whitespace from blank line
While the blank line serves a good purpose separating logical blocks, it should not contain whitespace characters.
-
+📝 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.
🧰 Tools
🪛 Ruff (0.8.0)
16-16: Blank line contains whitespace
Remove whitespace from blank line
(W293)
| @@ -22,7 +22,7 @@ def convert(graph_to_convert: AGraph | str | IO, layout_prog: str = "dot") -> st | |||
| graph_nodes: dict[str, dict] = {n: n.attr.to_dict() for n in graph.nodes_iter()} | |||
|
|
|||
| svg_graph = graph.draw(prog=layout_prog, format="svg") | |||
|
|
|||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove trailing whitespace from blank line
While the blank line provides good visual separation between initialization and processing, it should not contain whitespace characters.
-
+📝 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.
🧰 Tools
🪛 Ruff (0.8.0)
25-25: Blank line contains whitespace
Remove whitespace from blank line
(W293)
| def parse_svg_path(path_data): | ||
| # Extract all numbers from the path data | ||
| numbers = list(map(float, re.findall(r'-?\d+\.?\d*', path_data))) | ||
|
|
||
| # Separate x and y coordinates | ||
| x_coords = numbers[0::2] # Every other value starting from index 0 | ||
| y_coords = numbers[1::2] # Every other value starting from index 1 | ||
| # Calculate the bounding box | ||
| min_x = min(x_coords) | ||
| max_x = max(x_coords) | ||
| min_y = min(y_coords) | ||
| max_y = max(y_coords) | ||
|
|
||
| width = max_x - min_x | ||
| height = max_y - min_y | ||
|
|
||
| # Single x and y as the center of the bounding box | ||
| single_x = (min_x + max_x) / 2 | ||
| single_y = (min_y + max_y) / 2 | ||
|
|
||
| return single_x, single_y, width, height |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type hints and error handling to improve robustness.
The function needs several improvements for better maintainability and reliability:
-def parse_svg_path(path_data):
+def parse_svg_path(path_data: str) -> tuple[float, float, float, float]:
+ """Parse SVG path data and return center coordinates with dimensions.
+
+ Args:
+ path_data: SVG path data string containing coordinate values
+
+ Returns:
+ tuple of (center_x, center_y, width, height)
+
+ Raises:
+ ValueError: If path_data is empty or contains invalid numbers
+ """
# Extract all numbers from the path data
- numbers = list(map(float, re.findall(r'-?\d+\.?\d*', path_data)))
+ if not path_data:
+ raise ValueError("Empty path data")
+
+ number_matches = re.findall(r'-?\d+\.?\d*(?:e-?\d+)?', path_data)
+ if not number_matches:
+ raise ValueError("No valid numbers found in path data")
+
+ try:
+ numbers = [float(n) for n in number_matches]
+ except ValueError as e:
+ raise ValueError(f"Invalid number in path data: {e}")Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.8.0)
8-8: Missing return type annotation for public function parse_svg_path
(ANN201)
8-8: Missing type annotation for function argument path_data
(ANN001)
10-10: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
| def get_d(g: Element) -> str | None: | ||
| polygon_str = ET.tostring(g, encoding='unicode') | ||
| # Regular expression to find the <ns0:polygon> tag | ||
| polygon_elements = re.findall(r'<ns0:path[^>]*>', polygon_str) | ||
|
|
||
| # Join the results (in case there are multiple matches) | ||
| polygon_str = ''.join(polygon_elements) | ||
| match = re.search(r'd="([^"]+)"', polygon_str) | ||
| # If a match is found, format it as a dictionary | ||
| if match: | ||
| points = match.group(1) | ||
| x_coords, y_coords, width, height = parse_svg_path(points) | ||
| return x_coords, y_coords, width, height |
There was a problem hiding this comment.
💡 Codebase verification
Refactoring suggestion needs adjustment
The review comment's suggested changes need revision based on the verification results:
- The function
get_dis currently used in NodeFactory.py where it correctly unpacks 4 values (x_coords, y_coords, width, height) and the return values are used to create a Rect object. - The current return type annotation
str | Noneis incorrect as the function actually returns a tuple of coordinates and dimensions. - The function name
get_dshould remain as is since it refers to the SVG path's 'd' attribute, following SVG terminology.
def get_d(g: Element) -> tuple[float, float, float, float] | None:
"""Extract coordinates and dimensions from an SVG path element's 'd' attribute.
Args:
g: SVG Element containing a path
Returns:
tuple of (x_coords, y_coords, width, height) or None if no path found
"""
- polygon_str = ET.tostring(g, encoding='unicode')
- # Regular expression to find the <ns0:polygon> tag
- polygon_elements = re.findall(r'<ns0:path[^>]*>', polygon_str)
-
- # Join the results (in case there are multiple matches)
- polygon_str = ''.join(polygon_elements)
- match = re.search(r'd="([^"]+)"', polygon_str)
- # If a match is found, format it as a dictionary
- if match:
- points = match.group(1)
- x_coords, y_coords, width, height = parse_svg_path(points)
+ path = g.find(f".//{NS_SVG}path")
+ if path is not None and "d" in path.attrib:
+ points = path.attrib["d"]
+ try:
+ return parse_svg_path(points)
+ except ValueError as e:
+ logging.warning(f"Failed to parse path data: {e}")
+ return None
+ return NoneThe main issues to address are:
- Fix the incorrect return type annotation
- Add proper docstring
- Use XML parsing instead of string manipulation
- Add error handling for parse_svg_path
- Keep the original function name as it's domain-specific
🔗 Analysis chain
Refactor for robustness and clarity.
The current implementation has several issues:
- Converting XML to string for parsing is fragile and inefficient
- Function name doesn't reflect its return value
- Missing type hints and error handling
-def get_d(g: Element) -> str | None:
+def get_path_dimensions(g: Element) -> tuple[float, float, float, float] | None:
+ """Extract center coordinates and dimensions from an SVG path element.
+
+ Args:
+ g: SVG Element containing a path
+
+ Returns:
+ tuple of (center_x, center_y, width, height) or None if no path found
+ """
- polygon_str = ET.tostring(g, encoding='unicode')
- # Regular expression to find the <ns0:polygon> tag
- polygon_elements = re.findall(r'<ns0:path[^>]*>', polygon_str)
-
- # Join the results (in case there are multiple matches)
- polygon_str = ''.join(polygon_elements)
- match = re.search(r'd="([^"]+)"', polygon_str)
- # If a match is found, format it as a dictionary
- if match:
- points = match.group(1)
- x_coords, y_coords, width, height = parse_svg_path(points)
+ path = g.find(f".//{NS_SVG}path")
+ if path is not None and "d" in path.attrib:
+ points = path.attrib["d"]
+ try:
+ return parse_svg_path(points)
+ except ValueError as e:
+ logging.warning(f"Failed to parse path data: {e}")
+ return None
+ return NoneAlso, consider verifying the impact of these changes on the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all usages of get_d to ensure they handle the new return type
rg -l "get_d\(" | xargs ast-grep --pattern 'get_d($$$)'
Length of output: 57
Script:
#!/bin/bash
# Search for get_d function calls using ripgrep with context
rg -A 3 "get_d\("
Length of output: 719
Script:
#!/bin/bash
# Search for the function definition to verify its location
rg -A 5 "^def get_d"
Length of output: 475
Script:
#!/bin/bash
# Find all Python files that might import or use SVG.py
fd ".py$" | xargs rg "from.*SVG import|import.*SVG"
Length of output: 331
Script:
#!/bin/bash
# Check the usage context in NodeFactory.py to understand how the return values are used
rg -B 5 -A 10 "get_d\(" graphviz2drawio/mx/NodeFactory.py
Length of output: 933
Script:
#!/bin/bash
# Look for parse_svg_path function to understand the return type contract
rg -A 5 "def parse_svg_path"
Length of output: 464
🧰 Tools
🪛 Ruff (0.8.0)
55-55: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
57-57: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
60-60: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
63-66: Missing explicit return at the end of function able to return non-None value
Add explicit return statement
(RET503)
| @@ -68,6 +67,7 @@ def parse_nodes_edges_clusters( | |||
| else: | |||
| edges[edge.key_for_label] = edge | |||
| elif g.attrib["class"] == "cluster": | |||
|
|
|||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove unnecessary empty line
This empty line is not needed and affects code readability.
| @@ -15,6 +15,7 @@ | |||
| from .commented_tree_builder import COMMENT, CommentedTreeBuilder | |||
| from .CoordsTranslate import CoordsTranslate | |||
| from .Errors import MissingTitleError | |||
| import xml.etree.ElementTree as ET | |||
There was a problem hiding this comment.
Remove redundant import
This import is redundant as ElementTree is already imported on line 5. Additionally, the ET alias is never used in the code.
Apply this diff to remove the redundant import:
-import xml.etree.ElementTree as ET📝 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.
| import xml.etree.ElementTree as ET |
🧰 Tools
🪛 Ruff (0.8.0)
18-18: xml.etree.ElementTree imported but unused
Remove unused import: xml.etree.ElementTree
(F401)
| @@ -167,4 +171,4 @@ def __str__(self) -> str: | |||
| return self.value() | |||
|
|
|||
| def __repr__(self) -> str: | |||
| return self.value() | |||
| return self.value() | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add newline at end of file.
Add a trailing newline to follow Python coding standards.
- return self.value()
+ return self.value()
+📝 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.
| return self.value() | |
| return self.value() | |
🧰 Tools
🪛 Ruff (0.8.0)
174-174: No newline at end of file
Add trailing newline
(W292)
| @@ -8,6 +8,7 @@ | |||
| from graphviz2drawio.mx.Edge import Edge | |||
| from graphviz2drawio.mx.Node import Node | |||
| from graphviz2drawio.mx.Styles import Styles | |||
| from graphviz2drawio.models import SVG | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove unused import.
The SVG import from graphviz2drawio.models is not used in this file.
-from graphviz2drawio.models import SVG📝 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.
| from graphviz2drawio.models import SVG |
🧰 Tools
🪛 Ruff (0.8.0)
11-11: graphviz2drawio.models.SVG imported but unused
Remove unused import: graphviz2drawio.models.SVG
(F401)
| width = abs(text_offset.real) * 0.25 # Example calculation | ||
| height = abs(text_offset.imag) * 0.25 # Example calculation |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Document or refactor magic numbers.
The width and height calculations use a magic number (0.25) without explanation. Consider extracting this as a named constant or documenting why this specific scaling factor is used.
- width = abs(text_offset.real) * 0.25 # Example calculation
- height = abs(text_offset.imag) * 0.25 # Example calculation
+ SCALING_FACTOR = 0.25 # Factor for converting text offset to dimensions
+ width = abs(text_offset.real) * SCALING_FACTOR
+ height = abs(text_offset.imag) * SCALING_FACTOR📝 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.
| width = abs(text_offset.real) * 0.25 # Example calculation | |
| height = abs(text_offset.imag) * 0.25 # Example calculation | |
| SCALING_FACTOR = 0.25 # Factor for converting text offset to dimensions | |
| width = abs(text_offset.real) * SCALING_FACTOR | |
| height = abs(text_offset.imag) * SCALING_FACTOR |
| if (polygon := SVG.get_first(g, "path")) is not None: | ||
| x_coords, y_coords, width, height = SVG.get_d(polygon) | ||
| rect = Rect(x=x_coords,y=y_coords,width=width,height=height) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename variable polygon to path_element for clarity
The variable polygon is assigned a path element from the SVG. To improve code readability and prevent confusion, consider renaming polygon to path_element.
Apply this diff to rename the variable:
-if (polygon := SVG.get_first(g, "path")) is not None:
- x_coords, y_coords, width, height = SVG.get_d(polygon)
- rect = Rect(x=x_coords, y=y_coords, width=width, height=height)
+if (path_element := SVG.get_first(g, "path")) is not None:
+ x_coords, y_coords, width, height = SVG.get_d(path_element)
+ rect = Rect(x=x_coords, y=y_coords, width=width, height=height)📝 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.
| if (polygon := SVG.get_first(g, "path")) is not None: | |
| x_coords, y_coords, width, height = SVG.get_d(polygon) | |
| rect = Rect(x=x_coords,y=y_coords,width=width,height=height) | |
| if (path_element := SVG.get_first(g, "path")) is not None: | |
| x_coords, y_coords, width, height = SVG.get_d(path_element) | |
| rect = Rect(x=x_coords,y=y_coords,width=width,height=height) |
|
Any updates on this implementation? |
Summary by Sourcery
Add a function to parse SVG path data and calculate bounding box dimensions, and refactor NodeFactory to use path elements instead of polygon elements for SVG parsing.
New Features:
Enhancements:
Summary by CodeRabbit
New Features
Bug Fixes
Chores