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

feat: allow interaction handlers of any observable type #1261

Merged
merged 1 commit into from Feb 3, 2017

Conversation

Projects
None yet
3 participants
@kentcb
Copy link
Contributor

kentcb commented Feb 3, 2017

Fixes #1260.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 3, 2017

Coverage Status

Coverage increased (+0.01%) to 65.574% when pulling 50fe504 on kentcb:Issue1260 into baab571 on reactiveui:develop.

@ghuntley
Copy link
Member

ghuntley left a comment

  • Is this a breaking change and/or do we need to offer a migration path?
  • Documentation will need to be updated once this is merged.
@@ -200,14 +200,16 @@ public IDisposable RegisterHandler(Func<InteractionContext<TInput, TOutput>, Tas
/// <returns>
/// A disposable which, when disposed, will unregister the handler.
/// </returns>
public IDisposable RegisterHandler(Func<InteractionContext<TInput, TOutput>, IObservable<Unit>> handler)
public IDisposable RegisterHandler<TDontCare>(Func<InteractionContext<TInput, TOutput>, IObservable<TDontCare>> handler)

This comment has been minimized.

@ghuntley

ghuntley Feb 3, 2017

Member

Should we keep public IDisposable RegisterHandler(Func<InteractionContext<TInput, TOutput>, IObservable<Unit>> handler) as an alias method to the new signature but annotate it with [Obsolete("Please use RegisterHandler<T> instead?")] as a migration path? I want to stop making hard public API contract changes. Your thoughts?

@kentcb

This comment has been minimized.

Copy link
Contributor Author

kentcb commented Feb 3, 2017

@ghuntley it's not a breaking change in the sense that if people were accidentally calling the non-observable version because their observable was not Unit, this change will actually help them.

@ghuntley ghuntley added this to the vNext milestone Feb 3, 2017

@ghuntley
Copy link
Member

ghuntley left a comment

Alright. Can you please update the docs and once done, self-merge. Sometimes in our release notes we draft up something more verbose and human to explain the rational behind a new feature (i.e. advertise the benefits) - this change would be a good candidate.

@ghuntley ghuntley changed the title Allow interaction handlers of any observable type feature: allow interaction handlers of any observable type Feb 3, 2017

@kentcb

This comment has been minimized.

Copy link
Contributor Author

kentcb commented Feb 3, 2017

@ghuntley not sure which docs you mean? Nothing has changed from a consumer point of view apart from the API being more forgiving of the observable type. But the docs don't talk about that any way.

@ghuntley

This comment has been minimized.

Copy link
Member

ghuntley commented Feb 3, 2017

Apologies, an assumption on my part that documentation would need updating. If they don't, they don't. I've started thinking about making this a standard question when merging pull requests - does the doc need updating and if so please do it. I'm keen to merge the documentation repo back into the main RxUI repo at some stage this year so documentation gets versioned along side the code.

@ghuntley ghuntley merged commit b770d75 into reactiveui:develop Feb 3, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage increased (+0.01%) to 65.574%
Details

@kentcb kentcb changed the title feature: allow interaction handlers of any observable type feat: allow interaction handlers of any observable type Feb 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment