-
Notifications
You must be signed in to change notification settings - Fork 43
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
front: ensure destination is last in formatSuggestedViasToRowVias() #8037
Conversation
I tried isolating the logic in a function to avoid repetition, but ended up with something that was IMHO harder to read. (Maybe could have a function to do the |
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## dev #8037 +/- ##
============================================
- Coverage 28.12% 28.11% -0.01%
Complexity 2112 2112
============================================
Files 1296 1296
Lines 158647 158659 +12
Branches 3164 3164
============================================
- Hits 44614 44611 -3
- Misses 112131 112146 +15
Partials 1902 1902
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
013327b
to
979555f
Compare
(-1 || 0) is always evaluated to -1. We want to move the OP to the start of the array if we find it. If it's already at the start, the logic inside the conditional is a no-op.
Instead of toSpliced() + unshift(), we can use array destructuring to swap the first element with the one matching the origin.
The array has the type PathStep[], so not need to cast.
We were only handling the case where the path step is a UIC. Use matchPathStepAndOp() so that we handle all other cases (track offset, trigram, OP ID) correctly too.
Apply the same logic as the origin, but for the destination. See #8032 (comment)
979555f
to
c9c1832
Compare
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.
Lgtm and tested
See individual commits (some cleanups are also included).
See #8032 (comment)
Note, the first commit has been moved from #8032 to this PR to avoid merge conflicts.