Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add a fallback method #136

wants to merge 1 commit into from

Conversation

PhilipAxelrod
Copy link
Contributor

No description provided.

printOnFallback: Boolean = false): Stream[T] = {
var defaultValueIsFresh = true

val cancel1 = fallBack.foreach(_ => defaultValueIsFresh = false)
Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

@PhilipAxelrod PhilipAxelrod Dec 19, 2017

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

val cancelFunction = this.foreach(v => updateEvent.apply(condition(v)))

will break?

@codecov
Copy link

codecov bot commented Dec 18, 2017

Codecov Report

Merging #136 into master will decrease coverage by 0.48%.
The diff coverage is 0%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
...om/lynbrookrobotics/potassium/streams/Stream.scala 86.44% <0%> (-5.18%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a4440ad...40d7c4c. Read the comment docs.

@shadaj
Copy link
Contributor

shadaj commented Feb 15, 2018

@paxelord do we want to move forward with this PR or just drop and replace with the simpler approach we discussed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants