-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add a fallback method #136
base: master
Are you sure you want to change the base?
Conversation
printOnFallback: Boolean = false): Stream[T] = { | ||
var defaultValueIsFresh = true | ||
|
||
val cancel1 = fallBack.foreach(_ => defaultValueIsFresh = false) |
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.
As soon as this method returns, these variables are no longer needed and may be cleaned up. You should probably create a new class for a fallback stream that stores these listeners.
This is super janky, but will be fixed with the new stream tracking mechanism, which I hope to PRify this week.
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.
Wait, but they are needed though. It's to make sure that the foreach stuff is never cancelled
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 mean from the JVM perspective, those variables are not depended on by anyone and so would be cleaned out.
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.
Would that mean that this line in the code for creating events
potassium/core/shared/src/main/scala/com/lynbrookrobotics/potassium/streams/Stream.scala
Line 510 in 198a1f4
val cancelFunction = this.foreach(v => updateEvent.apply(condition(v))) |
will break?
Codecov Report
@@ Coverage Diff @@
## master #136 +/- ##
==========================================
- Coverage 64.92% 64.44% -0.49%
==========================================
Files 68 68
Lines 1340 1350 +10
Branches 131 129 -2
==========================================
Hits 870 870
- Misses 470 480 +10
Continue to review full report at Codecov.
|
@paxelord do we want to move forward with this PR or just drop and replace with the simpler approach we discussed? |
No description provided.