-
Notifications
You must be signed in to change notification settings - Fork 54
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
chore: Remove dead code from multi node #186
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #186 +/- ##
===========================================
- Coverage 57.43% 57.42% -0.01%
===========================================
Files 98 98
Lines 9643 9616 -27
===========================================
- Hits 5538 5522 -16
+ Misses 3491 3479 -12
- Partials 614 615 +1
|
if err := node.AddChild(field, plan); err != nil { | ||
return err | ||
} | ||
} else { // no multiscan, case B) |
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.
I couldn't spot how this case is possible - if you know, please let me know and I'll try test it
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.
Traced things through, and I can't see how it is possible either, good catch!
case mergeNode: | ||
if ms := node.Source(); s != nil { // yes, we have a multiscan node. Case A) |
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.
This actually looks like a typo ms := ...; s !=...
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.
👀, indeed it is, oddly enough, I checked it locally and all the tests still pass with the correction.
Can't tell if im confused or dumb. I first thought based on the issue and PR title you were adding tests to explicitly cover the |
Yeah, is a bit misleading - intention was to write more tests, but I couldn't find a way to hit the codeblocks with existing node |
We can certainly do more w.r.t unit testing the |
I can rename the PR. |
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
82d93f1
to
ee9e04c
Compare
ee9e04c
to
db82554
Compare
* Remove dead MultiNode merge code * Remove commented out code * Remove impossible code block
Closes #184
Couldn't find a way to re-open original PR: #185 (maybe permission thing, button was disabled).