Fix https://github.com/hbmartin/graphviz2drawio/issues/45 and support Images nodes #49
Conversation
Sourcery Code Quality Report✅ Merging this PR will increase code quality in the affected files by 3.47%.
Here are some functions in these files that still need a tune-up:
Legend and ExplanationThe emojis denote the absolute quality of the code:
The 👍 and 👎 indicate whether the quality has improved or gotten worse with this pull request. Please see our documentation here for details on how these metrics are calculated. We are actively working on this report - lots more documentation and extra metrics to come! Help us improve this quality report! |
laurierloi
force-pushed
the
svg-fix-nested
branch
2 times, most recently
from
9f28a0a
to
d3c5a89
Compare
Instead of looking only at direct childs of a node, search the complete tree starting at the node. This is particularly useful when we add fancy attributes, since they tend to add encompassing groups to the graph. It is pretty easy to test for the bug and how it can be reproduced. It's tracked in: hbmartin#45 I re-created a minimal fail case from hbmartin#45 (comment) ``` digraph { Pitas Pizza [ tooltip=Delicious ] Pitas -> Pizza } ``` Previous HEAD failed to parse this.
Image nodes are nice to have in some schematics. Because of the representation in mxGraph, it would be a pain to create them in the xml. Instead, this follows the least-effort methodology and replace the image by an equally sized rectangle. In drawio GUI, the image can then be switched back in place of the rectangle. As a reference on the mxImageShape if someone wants to dive deeper: https://jgraph.github.io/mxgraph/java/docs/com/mxgraph/shape/mxImageShape.html
laurierloi
force-pushed
the
svg-fix-nested
branch
from
d3c5a89
to
49b6ea9
Compare
|
Any luck with merging this? |
WalkthroughThe recent updates focus on enhancing SVG handling in the Changes
Sequence Diagram(s)Creating
|
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- graphviz2drawio/models/SVG.py (2 hunks)
- graphviz2drawio/mx/NodeFactory.py (2 hunks)
- test/directed/tooltip.gv.txt (1 hunks)
Files skipped from review due to trivial changes (1)
- test/directed/tooltip.gv.txt
Additional context used
Ruff
graphviz2drawio/models/SVG.py
12-14: Avoid specifying long messages outside the exception class
(TRY003)
13-13: Exception must not use an f-string literal, assign to variable first
Assign to variable; remove f-string literal
(EM102)
13-13: Unnecessary
listcomprehension (rewrite usinglist())Rewrite using
list()(C416)
13-13: Trailing comma missing
Add trailing comma
(COM812)
20-20: Blank line contains whitespace
Remove whitespace from blank line
(W293)
graphviz2drawio/mx/NodeFactory.py
36-36: Unnecessary
dictcall (rewrite as a literal)Rewrite as a literal
(C408)
Additional comments not posted (4)
graphviz2drawio/models/SVG.py (3)
3-4: LGTM!The function
svg_tagcorrectly constructs SVG tag names by prepending the SVG namespace.
30-31: LGTM!The function
is_tagcorrectly checks for the presence of a tag within an XML element.
34-40: LGTM!The function
hascorrectly checks for the presence of a tag within an XML element.graphviz2drawio/mx/NodeFactory.py (1)
58-59: LGTM!The function
from_svgcorrectly usesrect_from_imageto handle SVG images.
|
|
||
|
|
||
| def get_title(g: Element) -> str: | ||
| return get_first(g, "title").text | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Remove whitespace from blank line.
The blank line contains unnecessary whitespace.
-
+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
20-20: Blank line contains whitespace
Remove whitespace from blank line
(W293)
| if i.tag == target: | ||
| return i | ||
| raise RuntimeError( | ||
| f"Failed to find tag ({tag}) in {g}, contains {[i for i in g.iter()]}" |
There was a problem hiding this comment.
Trailing comma missing.
Add a trailing comma.
- raise RuntimeError(
- f"Failed to find tag ({tag}) in {g}, contains {[i for i in g.iter()]}"
- )
+ tags = list(g.iter())
+ message = f"Failed to find tag ({tag}) in {g}, contains {tags}"
+ raise RuntimeError(message)Committable suggestion was skipped due to low confidence.
Tools
Ruff
13-13: Exception must not use an f-string literal, assign to variable first
Assign to variable; remove f-string literal
(EM102)
13-13: Unnecessary
listcomprehension (rewrite usinglist())Rewrite using
list()(C416)
13-13: Trailing comma missing
Add trailing comma
(COM812)
Avoid using f-string literal in exception.
Assign the message to a variable first, then use it in the exception.
- raise RuntimeError(
- f"Failed to find tag ({tag}) in {g}, contains {[i for i in g.iter()]}"
- )
+ tags = [i for i in g.iter()]
+ message = f"Failed to find tag ({tag}) in {g}, contains {tags}"
+ raise RuntimeError(message)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.
| f"Failed to find tag ({tag}) in {g}, contains {[i for i in g.iter()]}" | |
| tags = [i for i in g.iter()] | |
| message = f"Failed to find tag ({tag}) in {g}, contains {tags}" | |
| raise RuntimeError(message) |
Tools
Ruff
13-13: Exception must not use an f-string literal, assign to variable first
Assign to variable; remove f-string literal
(EM102)
13-13: Unnecessary
listcomprehension (rewrite usinglist())Rewrite using
list()(C416)
13-13: Trailing comma missing
Add trailing comma
(COM812)
Unnecessary list comprehension.
Rewrite using list().
- raise RuntimeError(
- f"Failed to find tag ({tag}) in {g}, contains {[i for i in g.iter()]}"
- )
+ tags = list(g.iter())
+ message = f"Failed to find tag ({tag}) in {g}, contains {tags}"
+ raise RuntimeError(message)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.
| f"Failed to find tag ({tag}) in {g}, contains {[i for i in g.iter()]}" | |
| tags = list(g.iter()) | |
| message = f"Failed to find tag ({tag}) in {g}, contains {tags}" | |
| raise RuntimeError(message) |
Tools
Ruff
13-13: Exception must not use an f-string literal, assign to variable first
Assign to variable; remove f-string literal
(EM102)
13-13: Unnecessary
listcomprehension (rewrite usinglist())Rewrite using
list()(C416)
13-13: Trailing comma missing
Add trailing comma
(COM812)
| raise RuntimeError( | ||
| f"Failed to find tag ({tag}) in {g}, contains {[i for i in g.iter()]}" | ||
| ) |
There was a problem hiding this comment.
Avoid specifying long messages outside the exception class.
Long messages should be specified within the exception class.
- raise RuntimeError(
- f"Failed to find tag ({tag}) in {g}, contains {[i for i in g.iter()]}"
- )
+ tags = [i for i in g.iter()]
+ raise RuntimeError(f"Failed to find tag ({tag}) in {g}, contains {tags}")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.
| raise RuntimeError( | |
| f"Failed to find tag ({tag}) in {g}, contains {[i for i in g.iter()]}" | |
| ) | |
| tags = [i for i in g.iter()] | |
| raise RuntimeError(f"Failed to find tag ({tag}) in {g}, contains {tags}") |
Tools
Ruff
12-14: Avoid specifying long messages outside the exception class
(TRY003)
13-13: Exception must not use an f-string literal, assign to variable first
Assign to variable; remove f-string literal
(EM102)
13-13: Unnecessary
listcomprehension (rewrite usinglist())Rewrite using
list()(C416)
13-13: Trailing comma missing
Add trailing comma
(COM812)
| @@ -31,6 +31,14 @@ def rect_from_svg_points(self, svg): | |||
| height = test_height | |||
| return Rect(x=min_x, y=min_y, width=width, height=height) | |||
|
|
|||
| @staticmethod | |||
| def rect_from_image(attrib): | |||
| filtered = dict() | |||
There was a problem hiding this comment.
Unnecessary dict call.
Rewrite as a literal.
- filtered = dict()
+ filtered = {}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.
| filtered = dict() | |
| filtered = {} |
Tools
Ruff
36-36: Unnecessary
dictcall (rewrite as a literal)Rewrite as a literal
(C408)
|
Thank you! |
I encountered #45 converting a graph I built.
Added a commit which solves this issue by doing tree-search instead of direct-child search in SVG.py
Also added support (somewhat) for image nodes, which I also used in my graph.
They are parsed as plain rectangles with the same dimensions.
The idea is that the user can replace that rectangle with the desired image.
Adding the image directly to the XML seems tortuous, since they seem to have a custom encoding and we don't have access to the java library.
Summary by CodeRabbit
New Features
rect_from_imagemethod toNodeFactoryfor extracting attributes from SVG images.Refactor