Skip to content
This repository has been archived by the owner on Sep 20, 2021. It is now read-only.

Produce results in response of rule evaluations #95

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Produce results in response of rule evaluations #95

wants to merge 5 commits into from

Conversation

jubianchi
Copy link
Member

@jubianchi jubianchi commented Sep 14, 2016

See #94 for detailed information.

Todo:

  • Implement the Rules collection with an SplPriorityQueue
  • Use the hoa/heap library for the Rules collection
  • Implement the Then rule class (it returns a result when the rule validates the input)
  • Implement the ThenElse rule class (it returns a result when the rule validates the input and another when the rule does not validate the input)
  • Implement an algorithm to resolve rules dependencies (indeed, a rule can depend on another one). hoa/graph could be a good candidate for this kind of work.
  • Implement a rule() function to reference the result of one rule from another rule

Some notes about the technical choices:

  • I used some clones in the code: this is to guarantee immutability of the passed objects. For example the Hoa\Ruler\Rules::initializeRuler() may alter the ruler's asserter. Before doing this, we clone the ruler and then alter its content. Finally we return it to be used where needed.
  • I introduced a Hoa\Ruler\Result class (with a magic method: this will change). It also does a clone of the result in some cases: this is to avoid mutating the value returned by the rules.
  • This class (Hoa\Ruler\Result) is also here to return a structured result letting developers keep track of the returned value, the rule that produced it and its name in the collection

@Hywan
Copy link
Member

Hywan commented Sep 16, 2016

@jubianchi Tell me when you need a review.

@jubianchi
Copy link
Member Author

@Hywan you can start reviewing when you want: any comment is good.

There are still some things to do but I'd be glad to fix any issue you already saw ;)

@Hywan
Copy link
Member

Hywan commented Oct 14, 2016

ping?

@Pittiplatsch
Copy link

@jubianchi I am interested in this feature as well. Is there anything I can help you with?

@jubianchi
Copy link
Member Author

I can finish this issue if someone can give me:

  • clear feedbacks,
  • code review,
  • clear TODOs

With that I would be able to ship new code and fix everything you spotted in a single iteration. TBH I won't have time to come back here after every push so I'd rather do everything in a single iteration.

@mathroc
Copy link

mathroc commented Mar 17, 2017

Hi,

I took a look at the code because it seems similar to something I'm looking for. afaict, this PR provides classes to associate a result with a rule (Then & ThenElse) and a class to get the first valid result of a set of rules.
While that's useful it's not exactly what I'm after. I was hoping to have rules themselves return a result instead of a boolean, eg:

$rule = 'apply_discount(price, if(premium, 0.9, 1))';
$ruler = new Hoa\Ruler\Ruler();
$context         = new Hoa\Ruler\Context();
$context['user'] = true;
$context['product'] = '100';
// add apply_discount
assert($ruler->execute($rule, $context) === '90');

is this out of the scope of this PR ?

@ghost ghost assigned jubianchi Jan 5, 2018
@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.383% when pulling 2e8f513 on jubianchi:rule-results into 28174ca on hoaproject:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.383% when pulling 2e8f513 on jubianchi:rule-results into 28174ca on hoaproject:master.

@Hywan
Copy link
Member

Hywan commented Jan 22, 2018

@jubianchi Should we do another round review?

@jubianchi
Copy link
Member Author

@Hywan yes you can if you think this feature really makes sense in Ruler.

I was asking myself if it should stay here or go somewhere else...

BTW I updated the PR to use hoa/heap instead of SplHeap as requested in previous discussions.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

None yet

5 participants