-
Notifications
You must be signed in to change notification settings - Fork 198
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bugfix in manhattan routing if the first reference is a bend turn #2704
bugfix in manhattan routing if the first reference is a bend turn #2704
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @thomaslima - 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: 1 issue found
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
else: | ||
# Sometimes the path starts with a bend, | ||
# therefore the initial straight has zero length. | ||
# Adding it here so that the first port is correctly returned. | ||
wg_refs += [wg_ref] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (edge_case_not_handled): Consider handling the case where wg_ref
might be None
or invalid.
In the added else block, there's an assumption that wg_ref
is always a valid reference. It might be beneficial to add a check to ensure wg_ref
is not None
or does not fail other validity checks before adding it to wg_refs
.
else: | |
# Sometimes the path starts with a bend, | |
# therefore the initial straight has zero length. | |
# Adding it here so that the first port is correctly returned. | |
wg_refs += [wg_ref] | |
else: | |
if wg_ref is not None: | |
wg_refs += [wg_ref] |
else: | ||
# Sometimes the path starts with a bend, | ||
# therefore the initial straight has zero length. | ||
# Adding it here so that the first port is correctly returned. | ||
wg_refs += [wg_ref] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (testing): Missing test for the new condition handling zero-length straights.
The code change introduces handling for cases where the path starts with a bend, leading to a zero-length straight. It's crucial to add a unit test to verify that the first port is computed correctly in this scenario.
else: | |
# Sometimes the path starts with a bend, | |
# therefore the initial straight has zero length. | |
# Adding it here so that the first port is correctly returned. | |
wg_refs += [wg_ref] | |
else: | |
wg_refs.append(wg_ref) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2704 +/- ##
==========================================
- Coverage 71.78% 71.71% -0.07%
==========================================
Files 366 366
Lines 23755 23797 +42
Branches 3873 3876 +3
==========================================
+ Hits 17052 17067 +15
- Misses 5578 5603 +25
- Partials 1125 1127 +2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome! thank you Thomas!
I first observed this bug when calling
round_corners
on a path that is supposed to start with a bend. The first port is not computed properly because the first zero-length straight section is discarded.