Support invisible nodes and fix arrows #1
Conversation
|
This is a benchmark review for experiment This pull request was cloned from Experiment configurationreview_config:
# User configuration for the review
# - benchmark - use the user config from the benchmark reviews
# - <value> - use the value directly
user_review_config:
enable_ai_review: true
enable_rule_comments: false
enable_complexity_comments: benchmark
enable_security_comments: benchmark
enable_tests_comments: benchmark
enable_comment_suggestions: benchmark
enable_functionality_review: benchmark
enable_pull_request_summary: benchmark
enable_review_guide: benchmark
enable_approvals: true
ai_review_config:
# The model responses to use for the experiment
# - benchmark - use the model responses from the benchmark reviews
# - llm - call the language model to generate responses
model_responses:
comments_model: benchmark
comment_area_model: benchmark
comment_validation_model: benchmark
comment_suggestion_model: benchmark
complexity_model: benchmark
docstrings_model: benchmark
functionality_model: benchmark
security_model: benchmark
tests_model: benchmark
pull_request_summary_model: benchmark
review_guide_model: benchmark
# The pull request dataset to run the experiment on
pull_request_dataset:
- https://github.com/mraniki/iamlistening/pull/334
- https://github.com/mraniki/cefi/pull/475
- https://github.com/mraniki/MyLLM/pull/581
- https://github.com/mraniki/dxsp/pull/689
- https://github.com/jschalk/jaar/pull/239
- https://github.com/jschalk/jaar/pull/241
- https://github.com/jschalk/jaar/pull/242
- https://github.com/iptux-src/iptux/pull/620
- https://github.com/iptux-src/iptux/pull/622
- https://github.com/hacksider/Deep-Live-Cam/pull/46
- https://github.com/mnbf9rca/super_simple_tfl_status/pull/114
- https://github.com/mnbf9rca/super_simple_tfl_status/pull/115
- https://github.com/RockProfile/Django-deployment/pull/1
- https://github.com/hbmartin/graphviz2drawio/pull/83
- https://github.com/fairdataihub/codefair-app/pull/28
- https://github.com/totaldebug/atomic-calendar-revive/pull/1518
- https://github.com/iammiracle01/portfolio/pull/1
- https://github.com/iammiracle01/portfolio/pull/2
- https://github.com/iammiracle01/portfolio/pull/3
- https://github.com/supermario-ai/gpt-crawler/pull/1
- https://github.com/hbmartin/graphviz2drawio/pull/84
- https://github.com/hbmartin/graphviz2drawio/pull/85
- https://github.com/dreamerminsk/tasked/pull/85
- https://github.com/dreamerminsk/tasked/pull/86
- https://github.com/dreamerminsk/tasked/pull/84
- https://github.com/haalasz/fm-tools/pull/9
- https://github.com/haalasz/fm-tools/pull/10
- https://github.com/iptux-src/iptux/pull/619
- https://github.com/code-Harsh247/medsymptom/pull/1
- https://github.com/code-Harsh247/medsymptom/pull/2
- https://github.com/cpp-lln-lab/bidspm/pull/1263
- https://github.com/cpp-lln-lab/bidspm/pull/1264
- https://github.com/cpp-lln-lab/bidspm/pull/1265
- https://github.com/luiscarlop/Judge_AI/pull/22
- https://github.com/NoNormalCreeper/nonebot_plugin_wolfram/pull/6
- https://github.com/osama1998H/kalima/pull/39
- https://github.com/osama1998H/kalima/pull/40
- https://github.com/osama1998H/kalima/pull/41
- https://github.com/jackdewinter/pymarkdown/pull/1131
- https://github.com/Eliver-Salazar/PED/pull/12
- https://github.com/NextAlone/Nagram/pull/40
- https://github.com/strawberry-graphql/strawberry-django/pull/575
- https://github.com/strawberry-graphql/strawberry/pull/3558
- https://github.com/strawberry-graphql/strawberry/pull/3559
- https://github.com/Ruin2121/yahooquery/pull/9
- https://github.com/gdsfactory/gdsfactory/pull/2951
- https://github.com/gdsfactory/gdsfactory/pull/2954
- https://github.com/gdsfactory/gdsfactory/pull/2956
- https://github.com/gdsfactory/gdsfactory/pull/2957
- https://github.com/gdsfactory/cspdk/pull/51
review_comment_labels:
- label: correct
question: Is this comment correct?
- label: helpful
question: Is this comment helpful?
- label: comment-type
question: Is the comment type correct?
- label: comment-area
question: Is the comment area correct?
- label: llm-test
question: |
What type of LLM test could this comment become?
- 👍 - this comment is really good/important and we should always make it
- 👎 - this comment is really bad and we should never make it
- no reaction - don't turn this comment into an LLM test
# Benchmark reviews generated by running
# python -m scripts.experiment benchmark <experiment_name>
benchmark_reviews: []
|
Reviewer's Guide by SourceryThis pull request introduces support for invisible nodes in edges and fixes issues related to arrow rendering. It also includes enhancements to edge style handling, updates the README to reflect these changes, and adds a new test case to verify the correct handling of invisible nodes. File-Level Changes
Tips
|
There was a problem hiding this comment.
Hey @sourcery-ai-experiments-bot - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟡 Complexity: 3 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| def get_edge_source_target(self, edge: Edge) -> tuple[Node | None, Node | None]: | ||
| if edge.dir == DotAttr.BACK: | ||
| return self.nodes[edge.to], self.nodes[edge.fr] | ||
| return self.nodes[edge.fr], self.nodes[edge.to] | ||
|
|
||
| @staticmethod | ||
| def get_edge_style( | ||
| edge: Edge, # pytype: disable=invalid-annotation | ||
| source_geo: Rect | None, | ||
| target_geo: Rect | None, | ||
| ) -> str: | ||
| end_arrow = MxConst.BLOCK | ||
| end_fill = 1 | ||
| dashed = 1 if edge.line_style == DotAttr.DASHED else 0 | ||
| if edge.arrowtail is not None: | ||
| tail = edge.arrowtail | ||
| if edge.arrowtail[0] == DotAttr.NO_FILL: | ||
| end_fill = 0 | ||
| tail = edge.arrowtail[1:] | ||
| if tail == DotAttr.DIAMOND: | ||
| end_arrow = MxConst.DIAMOND | ||
| if edge.curve is not None: | ||
| style = Styles.EDGE.format( | ||
| end_arrow=end_arrow, | ||
| dashed=dashed, | ||
| end_fill=end_fill, | ||
| ) + (MxConst.CURVED if edge.curve.is_bezier else MxConst.SHARP) | ||
|
|
||
| if source_geo is not None: | ||
| exit_x, exit_y = source_geo.relative_location_along_perimeter( | ||
| edge.curve.start, | ||
| ) | ||
| style += f"exitX={exit_x};exitY={exit_y};" | ||
| if target_geo is not None: | ||
| entry_x, entry_y = target_geo.relative_location_along_perimeter( | ||
| edge.curve.end, | ||
| ) | ||
| style += f"entryX={entry_x};entryY={entry_y};" | ||
|
|
||
| return style | ||
|
|
||
| return Styles.EDGE.format( | ||
| end_arrow=end_arrow, | ||
| dashed=dashed, | ||
| end_fill=end_fill, | ||
| ) | ||
| return self.nodes.get(edge.to), self.nodes.get(edge.fr) |
There was a problem hiding this comment.
suggestion: Consider using Optional for type hinting.
Using Optional[Node] instead of Node | None can improve readability and consistency with common Python type hinting practices.
| def get_edge_source_target(self, edge: Edge) -> tuple[Node | None, Node | None]: | |
| if edge.dir == DotAttr.BACK: | |
| return self.nodes[edge.to], self.nodes[edge.fr] | |
| return self.nodes[edge.fr], self.nodes[edge.to] | |
| @staticmethod | |
| def get_edge_style( | |
| edge: Edge, # pytype: disable=invalid-annotation | |
| source_geo: Rect | None, | |
| target_geo: Rect | None, | |
| ) -> str: | |
| end_arrow = MxConst.BLOCK | |
| end_fill = 1 | |
| dashed = 1 if edge.line_style == DotAttr.DASHED else 0 | |
| if edge.arrowtail is not None: | |
| tail = edge.arrowtail | |
| if edge.arrowtail[0] == DotAttr.NO_FILL: | |
| end_fill = 0 | |
| tail = edge.arrowtail[1:] | |
| if tail == DotAttr.DIAMOND: | |
| end_arrow = MxConst.DIAMOND | |
| if edge.curve is not None: | |
| style = Styles.EDGE.format( | |
| end_arrow=end_arrow, | |
| dashed=dashed, | |
| end_fill=end_fill, | |
| ) + (MxConst.CURVED if edge.curve.is_bezier else MxConst.SHARP) | |
| if source_geo is not None: | |
| exit_x, exit_y = source_geo.relative_location_along_perimeter( | |
| edge.curve.start, | |
| ) | |
| style += f"exitX={exit_x};exitY={exit_y};" | |
| if target_geo is not None: | |
| entry_x, entry_y = target_geo.relative_location_along_perimeter( | |
| edge.curve.end, | |
| ) | |
| style += f"entryX={entry_x};entryY={entry_y};" | |
| return style | |
| return Styles.EDGE.format( | |
| end_arrow=end_arrow, | |
| dashed=dashed, | |
| end_fill=end_fill, | |
| ) | |
| return self.nodes.get(edge.to), self.nodes.get(edge.fr) | |
| def get_edge_source_target(self, edge: Edge) -> tuple[Optional[Node], Optional[Node]]: | |
| if edge.dir == DotAttr.BACK: | |
| return self.nodes.get(edge.to), self.nodes.get(edge.fr) | |
| return self.nodes.get(edge.fr), self.nodes.get(edge.to) |
There was a problem hiding this comment.
What type of LLM test could this comment become?
- 👍 - this comment is really good/important and we should always make it
- 👎 - this comment is really bad and we should never make it
- no reaction - don't turn this comment into an LLM test
| @@ -1,10 +1,18 @@ | |||
| _whitelist_attrs = ["dir"] | |||
There was a problem hiding this comment.
suggestion: Consider renaming _whitelist_attrs to _allowed_attrs.
The term 'whitelist' can be replaced with 'allowed' to use more inclusive language. Consider renaming _whitelist_attrs to _allowed_attrs.
| _whitelist_attrs = ["dir"] | |
| _allowed_attrs = ["dir"] |
There was a problem hiding this comment.
What type of LLM test could this comment become?
- 👍 - this comment is really good/important and we should always make it
- 👎 - this comment is really bad and we should never make it
- no reaction - don't turn this comment into an LLM test
| @@ -173,6 +173,15 @@ def test_subgraph_and_colors(): | |||
| assert "clust2" in xml | |||
|
|
|||
|
|
|||
| def test_invisible(): | |||
There was a problem hiding this comment.
suggestion (testing): Consider adding more assertions to test edge cases.
The test_invisible function currently checks for a specific string in the XML. It would be beneficial to add more assertions to cover additional edge cases, such as verifying the absence of visible nodes and ensuring that other attributes are correctly set.
| def test_invisible(): | |
| def test_invisible(): | |
| file = "test/directed/invisible.gv.txt" | |
| xml = graphviz2drawio.convert(file) | |
| assert "clust2" in xml | |
| assert "visible" not in xml | |
| assert "<node" not in xml | |
| assert "<edge" not in xml |
There was a problem hiding this comment.
What type of LLM test could this comment become?
- 👍 - this comment is really good/important and we should always make it
- 👎 - this comment is really bad and we should never make it
- no reaction - don't turn this comment into an LLM test
|
|
||
| root = ElementTree.fromstring(xml) | ||
| check_xml_top(root) | ||
| assert ';exitX=1.0000;exitY=0.5839;"' in xml |
There was a problem hiding this comment.
suggestion (testing): Use more descriptive assertion messages.
Adding descriptive messages to assertions can help quickly identify the cause of a test failure. For example: assert ';exitX=1.0000;exitY=0.5839;"' in xml, 'Invisible node exit coordinates not found in XML'.
| assert ';exitX=1.0000;exitY=0.5839;"' in xml | |
| assert ';exitX=1.0000;exitY=0.5839;"' in xml, 'Invisible node exit coordinates not found in XML' |
There was a problem hiding this comment.
What type of LLM test could this comment become?
- 👍 - this comment is really good/important and we should always make it
- 👎 - this comment is really bad and we should never make it
- no reaction - don't turn this comment into an LLM test
| file = "test/directed/invisible.gv.txt" | ||
| xml = graphviz2drawio.convert(file) | ||
|
|
||
| root = ElementTree.fromstring(xml) |
There was a problem hiding this comment.
suggestion (testing): Check for the correct handling of invisible edges.
Consider adding assertions to verify that invisible edges are handled correctly. This could include checking that the edges are present in the XML but not rendered, and that their attributes are set as expected.
| root = ElementTree.fromstring(xml) | |
| root = ElementTree.fromstring(xml) | |
| invisible_edges = root.findall(".//edge[@style='invis']") | |
| assert len(invisible_edges) > 0 | |
| for edge in invisible_edges: | |
| assert edge.get('rendered') == 'false' |
There was a problem hiding this comment.
What type of LLM test could this comment become?
- 👍 - this comment is really good/important and we should always make it
- 👎 - this comment is really bad and we should never make it
- no reaction - don't turn this comment into an LLM test
| if search < self.x: | ||
| return 0 | ||
| if search > self.x + self.width: | ||
| return 1 | ||
| ratio = (search - self.x) / self.width | ||
| return self._approx(ratio, 0.5, 0.1) |
There was a problem hiding this comment.
issue (complexity): Consider keeping the _approx method and adding type annotations for clarity.
The new code is more complex and loses some of the original functionality. Specifically:
- The
_approxmethod, which provided a way to approximate the ratio to a center value within a delta, has been removed. This might lead to less precise results inx_ratioandy_ratiomethods. - While type annotations are a good practice, they add a layer of complexity if the team is not familiar with them. However, this is a minor point compared to the loss of functionality.
- The original code had a clear separation of concerns with the
_approxmethod, making it easier to understand and maintain. The new code inlines the ratio calculation, which can make it harder to modify or extend in the future.
To address these issues, consider keeping the _approx method and adding type annotations for clarity:
class Rectangle:
def __init__(self, x: float, y: float, width: float, height: float, image: str) -> None:
# x,y is the top left corner
self.x = x
self.y = y
self.width = width
self.height = height
self.bottom = y + height
self.right = x + width
self.image = image
def x_ratio(self, search: float) -> float:
if search < self.x:
return 0
if search > self.x + self.width:
return 1
ratio = (search - self.x) / self.width
return self._approx(ratio, 0.5, 0.1)
def y_ratio(self, search: float) -> float:
if search < self.y:
return 0
if search > self.y + self.height:
return 1
ratio = (search - self.y) / self.height
return self._approx(ratio, 0.5, 0.1)
@staticmethod
def _approx(value: float, center: float, delta: float) -> float:
if abs(value - center) < delta:
return center
return value
def to_dict_str(self) -> dict[str, str]:
return {
"x": str(self.x),
"y": str(self.y),
"width": str(self.width),
"height": str(self.height),
}
def closest_point_along_perimeter(self, x: float, y: float) -> tuple[float, float]:
x = clamp(x, self.x, self.right)
y = clamp(y, self.y, self.bottom)
return x, yThis approach maintains the original functionality and readability while adding type annotations for clarity.
There was a problem hiding this comment.
What type of LLM test could this comment become?
- 👍 - this comment is really good/important and we should always make it
- 👎 - this comment is really bad and we should never make it
- no reaction - don't turn this comment into an LLM test
| self.labels = labels | ||
|
|
||
| def get_edge_style( |
There was a problem hiding this comment.
issue (complexity): Consider simplifying the code by reducing methods and parameters.
The new code introduces additional complexity that could be simplified. Here are some points to consider:
-
Increased Number of Methods: The new methods (
get_edge_styleand_get_arrow_shape_and_fill) add complexity without significantly improving modularity. They are tightly coupled with the internal state of theEdgeclass. -
Additional Parameters: The
get_edge_stylemethod introducessource_geoandtarget_geoparameters, increasing cognitive load. -
Conditional Logic: The new methods contain multiple layers of conditional logic, making the code harder to follow and maintain.
-
Duplication of Logic: There is some duplication in how styles are formatted and how arrow shapes and fills are determined, which could lead to maintenance issues.
-
Increased Dependencies: The new code introduces dependencies on additional modules (
Rect,MxConst, andStyles), increasing complexity and making it harder to test in isolation.
Consider simplifying the code by removing unnecessary parameters, reducing nested conditionals, and consolidating style formatting logic. This will make the code easier to read, understand, and maintain.
There was a problem hiding this comment.
What type of LLM test could this comment become?
- 👍 - this comment is really good/important and we should always make it
- 👎 - this comment is really bad and we should never make it
- no reaction - don't turn this comment into an LLM test
| EDGE_LABEL = ( | ||
| "edgeLabel;html=1;align=center;verticalAlign=bottom;resizable=0;points=[];" | ||
| ) | ||
| EDGE_INVIS = "rounded=1;html=1;exitX={exit_x:.3g};exitY={exit_y:.3g};jettySize=auto;curved={curved};endArrow={end_arrow};dashed={dashed};endFill={end_fill};" |
There was a problem hiding this comment.
issue (complexity): Consider refactoring to reuse the existing EDGE style and only override the necessary properties.
The new code introduces a new style EDGE_INVIS which has a lot of overlap with the existing EDGE style, leading to increased duplication and maintenance complexity. Consider refactoring to reuse the existing EDGE style and only override the necessary properties. This will reduce duplication, improve maintainability, and ensure consistency. Here's a suggested refactor:
from enum import Enum
from . import Shape
class Styles(Enum):
NODE = "verticalAlign=top;align=left;overflow=fill;html=1;rounded=0;shadow=0;comic=0;labelBackgroundColor=none;strokeColor={stroke};strokeWidth=1;fillColor={fill};"
EDGE = "html=1;endArrow={end_arrow};dashed={dashed};endFill={end_fill};startArrow={start_arrow};startFill={start_fill};"
EDGE_LABEL = "edgeLabel;html=1;align=center;verticalAlign=bottom;resizable=0;points=[];"
TEXT = "margin:0px;text-align:{align};{margin};font-size:{size}px;font-family:{family};color:{color};"
ELLIPSE = "ellipse;" + NODE
CIRCLE = "ellipse;aspect=fixed;" + NODE
HEXAGON = "shape=hexagon;perimeter=hexagonPerimeter2;" + NODE
EGG = "shape=mxgraph.flowchart.display;direction=south;" + NODE
TRIANGLE = "triangle;direction=north;" + NODE
LINE = "line;strokeWidth=2;verticalAlign=bottom;labelPosition=center;verticalLabelPosition=top;align=center;" + NODE
@staticmethod
def edge_invis(exit_x, exit_y, curved, end_arrow, dashed, end_fill):
return f"rounded=1;html=1;exitX={exit_x:.3g};exitY={exit_y:.3g};jettySize=auto;curved={curved};" + Styles.EDGE.format(
end_arrow=end_arrow, dashed=dashed, end_fill=end_fill, start_arrow="", start_fill=""
)This approach keeps the code DRY (Don't Repeat Yourself) and makes it more modular and maintainable.
There was a problem hiding this comment.
What type of LLM test could this comment become?
- 👍 - this comment is really good/important and we should always make it
- 👎 - this comment is really bad and we should never make it
- no reaction - don't turn this comment into an LLM test
Addresses hbmartin#67
Summary by Sourcery
This pull request introduces support for invisible nodes in edge handling and fixes issues with arrow rendering. It includes refactoring of edge style generation and updates to the README and test cases.
Summary by CodeRabbit
New Features
EDGE_INVISattribute for invisible edges.Bug Fixes
Tests
test_invisiblefunction to validate Graphviz to Draw.io XML conversion.Documentation
Enhancements
Refactor
Rect.pyfor ratio calculations.DotAttr.py.