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

ExecuteAllRulesAsync should support a CancellationToken as argument #609

Open
ErikApption opened this issue Jun 11, 2024 · 18 comments
Open

Comments

@ErikApption
Copy link

ErikApption commented Jun 11, 2024

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
Copy link

asulwer commented Jun 20, 2024

working on this in my fork

@asulwer
Copy link

asulwer commented Jun 20, 2024

correction, is working with all examples containing an example

@asulwer
Copy link

asulwer commented Jun 23, 2024

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

@ErikApption
Copy link
Author

Awesome - are you planning to submit a PR?

@asulwer
Copy link

asulwer commented Jun 24, 2024

@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
Copy link

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

@asulwer
Copy link

asulwer commented Jun 25, 2024

@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
Copy link

/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
Copy link

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
#604 (comment)

@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
Copy link

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

@RenanCarlosPereira
Copy link

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
Copy link

asulwer commented Jun 26, 2024

@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
Copy link

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
Copy link

@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
Copy link
Author

ErikApption commented Jun 26, 2024

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
Copy link

asulwer commented Jun 26, 2024

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
Copy link

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

@RenanCarlosPereira
Copy link

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:
asulwer#31

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants