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

Rework condition caching to support nested evaluation #90

Merged
merged 4 commits into from Apr 25, 2021

Conversation

ehuelsmann
Copy link
Member

Description

Prior to this change, only top-level condition outcomes were cached;
however, use of conditions isn't restricted to the top level, yet
with caching one would expect exactly a single condition evaluation
per block of cached conditions.

Note: this change is created for the purpose of discussion and therefor marked as Draft.

Type of change

I'm not sure if this is a bug fix or a new feature: we were supposed to have a condition cache, but nested condition evaluations weren't cached at all... (Which, given the lack of issues registered about this situation since its creation -- some 17 years ago -- might be an over-engineered function?)

  • Bug fix (non-breaking change which fixes an issue)
  • This change requires a documentation update

Checklist:

  • My code follows the style guidelines of this project, please see the contribution guidelines.
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes

@todo
Copy link

todo bot commented Jan 30, 2021

We may just want to pass the error up

https://github.com/jonasbn/perl-workflow/blob/f0d893a28964a61e3e22c23eaf6fca0d31976118/lib/Workflow/Condition.pm#L107-L112


This comment was generated by todo based on a TODO comment in f0d893a in #90. cc @ehuelsmann.

@todo
Copy link

todo bot commented Jan 30, 2021

**** We need a deprecation warning of some sort! This is

https://github.com/jonasbn/perl-workflow/blob/f0d893a28964a61e3e22c23eaf6fca0d31976118/lib/Workflow/Condition.pm#L363-L368


This comment was generated by todo based on a TODO comment in f0d893a in #90. cc @ehuelsmann.

@coveralls
Copy link

coveralls commented Jan 30, 2021

Coverage Status

Coverage decreased (-0.3%) to 81.955% when pulling f692b76 on ehuelsmann:cache-nested-conditions into f604055 on jonasbn:master.

@ehuelsmann ehuelsmann force-pushed the cache-nested-conditions branch 2 times, most recently from 57cda90 to 1be1846 Compare January 30, 2021 19:18
@ehuelsmann
Copy link
Member Author

discussed this topic on Slack with @oliwel last Friday. Although he didn't intimately review the change, he did get and endorsed the structure and direction of the change.

@ehuelsmann ehuelsmann marked this pull request as ready for review February 3, 2021 20:55
@jonasbn jonasbn added the enhancement Feature/improvement label Feb 3, 2021
@ehuelsmann
Copy link
Member Author

@jonasbn, based on your review feedback, I'll resolve the two "TODO" items that the bot complained about. That is, assuming you'll accept the basic idea behind this change.

@jonasbn
Copy link
Collaborator

jonasbn commented Mar 3, 2021

Hi @ehuelsmann

Sorry about the radio silence, life/work is busy at the moment. I have skimmed the PR and I can see that this review will require some time, so I am just writing to let you know that I am on it

Prior to this change, only top-level condition outcomes were cached;
however, use of conditions isn't restricted to the top level, yet
with caching one would expect exactly a single condition evaluation
per block of cached conditions.
@jonasbn jonasbn added this to the 1.54 milestone Apr 18, 2021
lib/Workflow/Condition.pm Outdated Show resolved Hide resolved
lib/Workflow/Condition.pm Outdated Show resolved Hide resolved
lib/Workflow/Condition.pm Outdated Show resolved Hide resolved
lib/Workflow/Condition.pm Outdated Show resolved Hide resolved
lib/Workflow/Condition.pm Outdated Show resolved Hide resolved
lib/Workflow/Condition.pm Outdated Show resolved Hide resolved
lib/Workflow/Condition.pm Outdated Show resolved Hide resolved
Copy link
Collaborator

@jonasbn jonasbn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am all for improvements in caching, the implementation looks promising, but is is extensive and I am not sure if I complete get all the details.

I do find the term "opposite" confusing when reading the code.

Please evaluate my comments, they are as always by no means dictation

@ehuelsmann ehuelsmann requested a review from jonasbn April 18, 2021 19:28
Copy link
Collaborator

@jonasbn jonasbn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@jonasbn jonasbn merged commit 33c4ab1 into perl-workflow:master Apr 25, 2021
@ehuelsmann ehuelsmann deleted the cache-nested-conditions branch August 3, 2021 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature/improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants