search

microsoft / RulesEngine

A Json based Rules Engine with extensive Dynamic expression support
https://microsoft.github.io/RulesEngine/
MIT License
3.47k stars 528 forks source link

ExecuteAllRulesAsync should support a CancellationToken as argument #609

Open ErikApption opened 3 weeks ago

ErikApption commented 3 weeks ago

The rule engine doesn't have any way to include proper cancellation during flows. ExecuteAllRulesAsync should be able to have an option CancellationToken as argument that would cancel the rule execution when necessary.

asulwer commented 1 week ago

working on this in my fork

asulwer commented 1 week ago

correction, is working with all examples containing an example

asulwer commented 1 week ago

the solution for this was to remove params from the method parameters which is a breaking change. i notated this in my fork

ErikApption commented 1 week ago

Awesome - are you planning to submit a PR?

asulwer commented 1 week ago

@ErikApption no, the original project is no longer supported. the original creator no longer works for microsoft, so this is dead. my fork will be supported, but i am moving in a new direction.

Supported Fork

timophe-91 commented 1 week ago

It's on my Forks roadmap and I'm trying to stay with less breaking changes. but @asulwer does a great job too.

asulwer commented 1 week ago

@timophe-91 i do not have any breaking changes. all original code has been labeled as obsolete. new methods were added with names chosen to correctly identify what they are meant to do. basically wrappers for existing code. all original issues have been fixed. i suggest sticking to one fork that is being updated. no fork will be taken seriously if everyone makes changes to just theirs.

timophe-91 commented 1 week ago

/microsoft/RulesEngine/issues/604#issuecomment-2184209616 @asulwer

my planned direction will continue to make large breaking changes. my fork wasn't to push you out, please do not think that. issues are being resolved but not closed, just doing what i can.

So i'm expecting your fork to have a lot of breaking changes of what you have written.

RenanCarlosPereira commented 1 week ago

hey guys, @timophe-91 and @asulwer we need to implement everything using PRs, that way we can keep the organization and changes going on. I don't want to break anybody you know, seems like a lot of people are using this project.

the idea is to fix the bugs, yes, and later on introduce breaking changes, not something for right now.

in respect of the creator @abbasc52 https://github.com/microsoft/RulesEngine/issues/604#issuecomment-2184408178

@asulwer if you want a contributor, we need to set up the PR process that was in place, building testing updating the CHANGELOG etc.

let me know if so I can join as a contributor unless you are moving to a new direction in your fork.

timophe-91 commented 1 week ago

@RenanCarlosPereira PRs are already implemented in my fork Main it PR only.

RenanCarlosPereira commented 1 week ago

ok, so let's define what fork we are going to use: my conditions are pretty straightforward.

@asulwer implemented some bug fixes in his fork. we can get there as contributors if the PR process and releases are in place. Otherwise, it will be hard to maintain and the project will lose credibility.

let me know what you guys think about it.

asulwer commented 1 week ago

@RenanCarlosPereira if you create a PR no one exists, in the main fork, to merge anything. all of the contributors that worked for Microsoft no longer do, they have the power to merge, thus my fork.

if anyone adds something to their fork i will include it in mine. that direction will be a lot of work, i know, but we need consistency. My organization is large and we are all working towards this over here. i welcome contributions in my fork.

timophe-91 commented 6 days ago

Thank you for your insights on different development paths and potential breaking changes. In my particular situation, I've found that simply updating dependencies addressed the production issues I was encountering. For now, I'll be proceeding with my existing fork to maintain stability and meet my immediate needs. I'm open to revisiting this decision in the future if circumstances change.

RenanCarlosPereira commented 6 days ago

@RenanCarlosPereira if you create a PR no one exists, in the main fork, to merge anything. all of the contributors that worked for Microsoft no longer do, they have the power to merge, thus my fork.

if anyone adds something to their fork i will include it in mine. that direction will be a lot of work, i know, but we need consistency. My organization is large and we are all working towards this over here. i welcome contributions in my fork.

Hi @asulwer,

I understand your point, but I didn't see any PRs in your fork. It would be great if we could manage everything through PRs in your fork. This way, we can ensure that your fork passes all the tests when merging to the master branch. Additionally, if we want to gain the trust of the community, this approach provides clear modifications so everyone can see the library evolving transparently. What do you think?

Thanks!

ErikApption commented 6 days ago

I had not realized that the package was not supported anymore. Thinking this one could use a new home (github org), and the direction should be discussed in the issues. @RenanCarlosPereira sounds like your version could be a minor version bump whereas @asulwer your version could be a major version bump. imo it is a shame that your work is buried into multiple forks

Any chances one of you can publish a nuget package? We've looked at multiple rule engine packages and we do like the json format of this one. NRules is good but we thought its Json syntax was complicated.

asulwer commented 6 days ago

Sorry for being a little lost there! Yes i will add PR's moving forward. i do have a nuget package pushed for all changes made. nothing breaking. original code exists, just made obsolete in favor of methods that have better descriptions. the code is a drop in replacement for the original fork, but your compiler will complete about the obsolete functions

timophe-91 commented 6 days ago

@ErikApption if you just need a dependency Update rn there is a nuget published at my fork.

RenanCarlosPereira commented 2 days ago

hey @timophe-91 I've implemented that into @asulwer fork if you want to include that implementation in your fork too: basically I introduce another overload so we can pass the cancellation token without breaking what people are using already.

then you can access the cancellation token in the context of the ActionBase or Action.

here is the PR that added some test to it: https://github.com/asulwer/RulesEngine/pull/31