Skip to content

Conversation

ehuelsmann
Copy link
Member

Description

Declaring the Workflow::Validator an interface contract as opposed to a
base class for validators allows any class to implement validations. This
addresses a long-standing desire of the OpenXPKI project to consolidate
the Condition and Validator concepts - in order to be able to share code
between the two - without actually consolidating them.

Closes #137

Type of change

  • New feature (non-breaking change which adds functionality)
  • Breaking change

This is really meant to be a documentation-only change. However, it didn't actually pan out that way, because of removing both the documentation and implementation of the _init() method: there's no place for a "dummy" method in an interface definition. Also, it doesn't make sense from an OOP perspective to have that method. Instead, a newly declared init() method should be calling its parent's init() as with regular OOP patterns.

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

This is a good point: should we add tests which implement a validator as a blessed ref not derived from Workflow::Validator or Workflow::Base?

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Declaring the Workflow::Validator an interface contract as opposed to a
base class for validators allows *any* class to implement validations. This
addresses a long-standing desire of the OpenXPKI project to consolidate
the Condition and Validator concepts - in order to be able to share code
between the two - without actually consolidating them.
@ehuelsmann ehuelsmann added this to the 2.0 milestone Feb 18, 2022
@ehuelsmann ehuelsmann requested review from jonasbn and oliwel February 18, 2022 12:27
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 92.431% when pulling e9471a4 on validator-as-interface into 0138f62 on master.

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.

I believe we need to add Moo to the cpanfile

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.

LGTM

Please ignore my previous comment, I missed it was in the documentation part

@ehuelsmann
Copy link
Member Author

@jonasbn As far as I'm concerned, we're ready to merge this.

@oliwel
Copy link
Collaborator

oliwel commented Aug 6, 2023

I did not check the related code details from the general idea behind I really vote for this!

@ehuelsmann ehuelsmann merged commit 0ab9f2e into master Aug 6, 2023
@delete-merged-branch delete-merged-branch bot deleted the validator-as-interface branch August 6, 2023 11:06
@ehuelsmann
Copy link
Member Author

Merged based on all prior approvals.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Remove Workflow::Exception::Validation
4 participants