asulwer / RulesEngine

Rules Engine with extensive Dynamic expression support
MIT License
19 stars 1 forks source link

Breaking Change: `SuccessEvent` to `SuccessMessage` Transition #6

Closed RenanCarlosPereira closed 2 months ago

RenanCarlosPereira commented 3 months ago

Hey @asulwer,

I noticed a breaking change in your branch related to the transition from SuccessEvent to SuccessMessage. This change affects the following file:

Action Required

  1. Update the CHANGELOG:

    • Document the transition from SuccessEvent to SuccessMessage. (I am not sure why we are making this change, so please provide a brief explanation if possible.)
  2. Update Documentation:

    • The following documentation files need to be updated to reflect this change:
      • Getting-Started.md
      • index.md
    • Additionally, ensure that any .json files within RulesEngine\RulesEngine.UnitTest that reference SuccessEvent are updated accordingly.

Next Steps

We need to decide whether to:

Your input and any additional context on why this change is being made would be greatly appreciated.

asulwer commented 3 months ago

I made the change to align with ErrorMessage. In my mind a lot of code seems to have been cobbled together with no real direction, naming methods especially. i brought up these issues years ago with a PR and nothing came of it. i will open a PR to change the places you have mentioned. the documentation is an entirely different beast. this project, in my opinion, has always suffered from poor documentation. we need to properly document all features, that was my next plan.

asulwer commented 3 months ago

i altered the changelog to include this issue for reference as to why it was changed. ErrorMessage is a message and so is SuccessMessage which is why i changed from SuccessEvent to SuccessMessage

asulwer commented 3 months ago

i am keeping this issue open just in case you have objections to my changes

asulwer commented 3 months ago

this became an issue, again, after latest merge

RenanCarlosPereira commented 3 months ago

Yeah, we will keep the way it is for now, we dont want to introduce breaking changes so early in the lib, the idea is wait for the people to migrate over, when everybody is confident on this repo we will be proposing changes that might have breaking changes 😇

asulwer commented 3 months ago

makes sense. i understand this now. i was just so excited to be able to make changes and have this moving along again.

abbasc52 commented 3 months ago

successMessage was an old field ( before output expression actions were introduced) .In a way I am not sure if it is used by community.

RenanCarlosPereira commented 3 months ago

Yeah thanks for your input @abbasc52

We will mark that as an obsolete and explain what to use instead, for example output expressions.

I don't think we need a new field for this propose, as you mentioned we have that output expression to cover that functionality 🙂

Marking as obsolete will give the community time to think about it

asulwer commented 3 months ago

removing the field is preferrable to renaming it? personally, at first, i used the field, but as @abbasc52 alluded to, actions are my preferred way to go