Make parameters in CommandFactory class consistent

[maxkoshevoi]

Description of Change

CommandFactory that was merged in #797 is not quite one that is described in issue or PR description.
When @brminnick separated CommandFactory into partial classes some changes to its API were made.
This PR is intended to restore some of them.

• Main issue: All overloads for Command were changed to accept the same parameters that Command constructors do which ruins the point of this class a little, since now some overloads of CommandFactory.Create accept different parameters from those that return AsyncCommand or AsyncValueCommand. Main benefit of this class is that it can automatically switch between ICommand implementations basically based on whether execute returns void or Task (or ValueTask)
• First parameter was renamed to execute in all overloads for all commands, which reintroduces ambiguity between async commands if more then two parameters are specified.

What was done:

• Made overloads of CommandFactory.Create for Command accept exactly the same parameters as overloads for async command do (except execute doesn't return task). This implementation of those overloads is different from ones that were in previous PR. Current behavior mimics Cammnd behavior for type validation (do nothing if execute type is not correct, and return false if canExecute type is not correct) instead of AsyncCommand behaviour (throw exception if type id not correct).
• Rename execute parameter for AsyncCommand to executeTask and AsyncValueCommand to executeValueTask. This is helpful to resolve ambiguity between them CommandFactory.Create(executeTask: async () => {}, null, null)
• Made canExecute parameter for Create<TExecute, TCanExecute> mandatory. It doesn't make sense to specify type for a parameter and not provide it.

Bugs Fixed

• Fixes [Enhancement] Add CommandFactory class #798

Behavioral Changes

CommandFactory.Create accepts the same parameters for all command types (except async execute for commands returns tasks, and nothing for regular one).

PR Checklist

• Has tests (if omitted, state reason in description)
• Has samples (if omitted, state reason in description)
• Rebased on top of main at time of PR
• Changes adhere to coding standard

• Updated documentation

[TheCodeTraveler]

Hi Maksym!

The changes I made to the previous PR were intentional.

Here's a deeper explanation about the updates I implemented:

Xamarin.Forms.Command

Xamarin.Forms.Command CommandFactory.Create is essentially a wrapper for Xamarin.Forms.Command.

As such, we shouldn't allow users to do anything with CommandFactory.Create that cannot already be done with Xamarin.Forms.Command, and vice versa.

1. Adhere to Xamarin.Forms.Command Constructors

The following CommandFactory.Create() methods are unnecessary because Xamarin.Forms.Command doesn't have a corresponding constructor.

I recommend first opening a PR and adding these constructors to Xamarin.Forms.Command; then we can include them in CommandFactory.Create.

public static Command Create<TExecute>(Action<TExecute> execute, Func<bool> canExecute);

public static Command Create<TExecute, TCanExecute>(Action<TExecute> execute, Func<TCanExecute, bool> canExecute);

public static Command Create<TExecute>(Action<TExecute> execute, Func<object, bool> canExecute);

2. Adhere to Xamarin.Forms.Command Validation

CommandFactory.Create should pass through values through to Xamarin.Forms.Command and Xamarin.Forms.Command<T>, relying on their existing validation of the parameters.

We should not be doing any validation on the execute or canExecute parameters that we pass to Xamarin.Forms.Command.

Therefore, the following helper methods are not necessary.

#region Helper methods to ensure Command behaviour is consistent with other commands

static Action<object> ConvertExecute(Action execute)
{
	if (execute == null)
		return null;

	return p => execute();
}

static Action<object> ConvertExecute<T>(Action<T> execute)
{
	if (execute == null)
		return null;

	return p => Execute(execute, p);
}

static void Execute<T>(Action<T> execute, object parameter)
{
	switch (parameter)
	{
		case T validParameter:
			execute(validParameter);
			break;

		case null when !typeof(T).GetTypeInfo().IsValueType:
			execute((T)parameter);
			break;

		case null:
		default:
			return;
	}
}

static Func<object, bool> ConvertCanExecute(Func<bool> canExecute)
{
	if (canExecute == null)
		return null;

	return _ => canExecute();
}

static Func<object, bool> ConvertCanExecute<T>(Func<T, bool> canExecute)
{
	if (canExecute == null)
		return null;

	return p => CanExecute(canExecute, p);
}

static bool CanExecute<T>(Func<T, bool> canExecute, object parameter) => parameter switch
{
	T validParameter => canExecute(validParameter),
	null when !typeof(T).GetTypeInfo().IsValueType => canExecute((T)parameter),
	_ => false,
};

#endregion

IAsyncCommand

The changes to the parameter names is not necessary.

You had originally changed the parameter names to prevent the call is ambiguous error that occurred when an anonymous async method was used with CommandFactory.Create

e.g. The following code was ambiguous between Task and ValueTask

CommandFactory.Create(async number => 
{ 
    if(number > 5)
        await Task.Delay(100);
});

I fixed this error in the previous PR by adding these additional CommandFactory.Create methods. These ensure that anonymous async methods will default to IAsyncCommand.

You can see this working in the Samples' AboutViewModel.

Here, we are initializing CommandFactory.Create using an async anonymous method without a call is ambiguous error:

SelectedContributorCommand = CommandFactory.Create(async () =>
{
    if (SelectedContributor == null)
        return;

    await Launcher.OpenAsync(SelectedContributor.HtmlUrl);
    SelectedContributor = null;
});

IAsyncValueCommand

Same as IAsyncCommand above, the changes to the parameter names is not necessary.

[maxkoshevoi]

@brminnick Thank you for detailed explanation!

Command constructors

I understand that adding Create overloads that wrap parameters into some custom logic is not the best idea. But that's to unify constructors across all command types.

PR into Xamarin repo is a good idea to solve this issue. Do you think they'll accept it?

Parameter names

Different execute parameter names are needed if user will specify more than two arguments. We don't have overloads for that.

Other

Are against making canExecute parameter for Create<TExecute, TCanExecute> mandatory?