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

Polish Signal based operators #779

Merged
merged 2 commits into from
Aug 7, 2017
Merged

Polish Signal based operators #779

merged 2 commits into from
Aug 7, 2017

Conversation

smaldini
Copy link
Contributor

@smaldini smaldini commented Aug 3, 2017

  • Turn Signal into interface
  • Remove MutableNextSignal
  • Precise FluxPeekStateful to FluxDoOnEach

@smaldini smaldini added the warn/api-change Breaking change with compilation errors label Aug 3, 2017
@smaldini smaldini added this to the 3.1.0.RC1 milestone Aug 3, 2017
- Turn Signal into interface
- Remove MutableNextSignal
- Precise FluxPeekStateful to FluxDoOnEach
Copy link
Member

@simonbasle simonbasle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you explain the rationale for this PR? seems like mutable signal and more generic FluxPeekStateful could be useful in avoiding creating too much garbage (wrapping every element in a new Signal)

@@ -69,4 +70,72 @@ public SignalType getType() {
return type;
}

//the base class defines equals and hashcode as final in order to allow

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

equals is not final and this is not a base class anymore, so remove broken comment ;)

}
return false;
}
//to implement additional state.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that was part of the comment of equals, remove it too

@smaldini
Copy link
Contributor Author

smaldini commented Aug 4, 2017

The rationale is simply shrinking surface API when possible due to its specificity and simplicity to reproduce. Also the operator was only used by doOnEach and if we happen to implement a doOnNext with state we'll revisit. In fact this operator should also be used by ParallelFlux too and eventually be fuseable.

@smaldini
Copy link
Contributor Author

smaldini commented Aug 4, 2017

Also DoOnEachSubscriber implement Signal directly to avoid allocations.

@smaldini smaldini merged commit 5233894 into master Aug 7, 2017
@smaldini smaldini deleted the polish-signal-operator branch August 7, 2017 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
warn/api-change Breaking change with compilation errors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants