reuse code; implement CancellationToken differently; sort array directly

[asulwer]

https://github.com/asulwer/RulesEngine/issues/27

[asulwer]

1. rearranged code
   • private methods under private region
   • public methods under public region
2. sort array instead of convert array to list, sort list, then convert back to list
3. removed params for methods that take CancellationToken as a parameter
4. removed duplicated code in some of the methods

[alexandreomiranda]

Hi guys, I've been watching this fork closely and we are willing to move from the original MS Rules Engine repo to this one. Good job on this new repo by the way. However, this change in the method signature will affect all existing calls we are making to the ExecuteAllRulesAsync method. That would be a significant breaking change on my side.

[techesd]

I moved to this fork since the original is not being maintained. With that, I'd not support breaking changes, as usual, only if it's really necessary which I don't think the case atm.

[asulwer]

@alexandreomiranda @techesd

this PR should not break existing calls to original methods. only if you are using the new methods that accept CancellationToken as a parameter. if those methods are called then, yes, your code would change

[asulwer]

methods that have parameter CancellationToken are newly added to this repository. This PR changed the signature of those two methods. all other code changes were to move methods around to places more appropriate for those methods

[RenanCarlosPereira]

@asulwer

If you change the method ExecuteAllRulesAsync from using params to a regular array and reorder the parameters, it will break existing code. For instance, suppose we have the original method:

ValueTask<List<RuleResultTree>> ExecuteAllRulesAsync(string workflowName, CancellationToken cancellationToken, params object[] inputs);

And it gets called like this:

await rulesEngine.ExecuteAllRulesAsync("exampleWorkflow", cancellationToken, input1, input2, input3);

In the new proposed method:

ValueTask<List<RuleResultTree>> ExecuteAllRulesAsync(string workflowName, object[] inputs, CancellationToken cancellationToken);

The call would need to change to:

await rulesEngine.ExecuteAllRulesAsync("exampleWorkflow", new object[] { input1, input2, input3 }, cancellationToken);

If the original code is not updated, it will break because the method signature and parameter order have changed. This makes the migration to the new version difficult and error-prone for users.

[RenanCarlosPereira]

Additionally, if we move the CancellationToken to the end, it will mix the signature with other overloads due to the params keyword, causing ambiguity. For example:

Original methods:

ValueTask<List<RuleResultTree>> ExecuteAllRulesAsync(string workflowName, CancellationToken cancellationToken, params object[] inputs);
ValueTask<List<RuleResultTree>> ExecuteAllRulesAsync(string workflowName, params object[] inputs);

If we change to:

ValueTask<List<RuleResultTree>> ExecuteAllRulesAsync(string workflowName, object[] inputs, CancellationToken cancellationToken);

Calling:

await rulesEngine.ExecuteAllRulesAsync("exampleWorkflow", input1, input2, cancellationToken);

This call could be confused with another overload:

ValueTask<List<RuleResultTree>> ExecuteAllRulesAsync(string workflowName, params object[] inputs);

This ambiguity can lead to compilation errors or unexpected behavior, making it crucial to keep CancellationToken in its original position.

[asulwer]

@RenanCarlosPereira yes that is true. those are the only two affected methods.

[asulwer]

the method that contains the two parameters params and CancellationToken are the concern here? no problem to put that back. i can see the need

[RenanCarlosPereira]

The problem is not the one with the cancellation token, the issue is the params, they will get mixed up, check the example I put in the comments above.

Even if we remove the ones with the cancellation token and remain the ones you introduced, it will cause the issue

The design of this API is strict, we shouldn't change this interface

No problem about implementing a new one... we can design a new contract with the new methods names, but changing what is there, really not a good idea

[asulwer]

@RenanCarlosPereira i am not changing what is already there. the methods containing the CancellationToken are newly added. regardless i have changed the parameter order for the method containing the params, to hopefully, satisfy anyone that is using that particular method.

[RenanCarlosPereira]

Yeah but you also changed the type man, that will also break.

That should keep the object[] Because rules engine accepts objects that are not necessary Parameters, that will also break

I recommend not to change the interface, unless you want to add new methods that will not mix with the existing ones

[asulwer]

the type didn't change, the order in the file changed.