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

feature: Allow InvokeCommand to use IReactiveCommand<T, TResult> #3681

Closed
wants to merge 1 commit into from
Closed

feature: Allow InvokeCommand to use IReactiveCommand<T, TResult> #3681

wants to merge 1 commit into from

Conversation

SuperJMN
Copy link

What kind of change does this PR introduce?
Use interface intead of implementation.

What is the current behavior?

InvokeCommand cannot be used with instances of IReactiveCommand<T, TResult>

What is the new behavior?

InvokeCommand can be used with instances of IReactiveCommand<T, TResult>

What might this PR break?

Nothing.

Please check if the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

Other information:
Minor change. Should be fine.

@glennawatson glennawatson changed the title Use IReactiveCommand<T, TResult> feature: Allow InvokeCommand to use IReactiveCommand<T, TResult> Nov 26, 2023
@SuperJMN SuperJMN marked this pull request as draft November 26, 2023 23:17
@SuperJMN
Copy link
Author

Oh, build failures kicked in. I'll take a look later.

@glennawatson
Copy link
Contributor

Was just discussing this one between myself and @ChrisPulman

I think this one might have to be a reject, due to the nature of C#

If the parameters only change based on having two interfaces, C# can't handle it and will cause ambiguity

The solutions you could potentially do is create a new method InvokeReactiveCommand( but that gets away from the point we don't want users if possible ICommand except when required by their UI framework.

Also then there is a argument why aren't you using ReactiveCommandBase if you're using a IReactiveCommand

I think it was definitely a nice to have one if the compiler could handle the two interface declarations and would of supported it going in, but unfortunately in this case I don't want to have method name changes imposed on existing code base etc.

Seemed simple but unfortunately not.

@SuperJMN SuperJMN deleted the generalize-invoke-command branch November 27, 2023 08:36
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants