Skip to content

Conversation

@pszulczewski
Copy link
Collaborator

This is a proposal to implement CheckType as abstract class and make validate() & evaluate() abstract methods.
This ensures that every child CheckType must have validate() & evaluate() methods implemented.

Tests show that TypeError exception is raised when an object is instantiated for a child class without validate() & evaluate() methods implemented.

[
("exact_match", ExactMatchType),
("tolerance", ToleranceType),
("parameter_match", ParameterMatchType),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Parameter and regex were missing here.


def evaluate(self, value_to_compare: Any, **kwargs) -> Tuple[Dict, bool]:
@abstractmethod
def evaluate(self, *args, **kwargs) -> Tuple[Dict, bool]:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed that to be more generic, so code validation doesn't highlight it as method signature discrepancy in child classes.

Copy link
Contributor

@chadell chadell left a comment

Choose a reason for hiding this comment

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

LGTM

@pszulczewski pszulczewski marked this pull request as ready for review February 8, 2022 19:38
Copy link
Collaborator

@lvrfrc87 lvrfrc87 left a comment

Choose a reason for hiding this comment

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

Good one! That should overcome the concern we had about the implementation of those methods from future contributors.

@lvrfrc87 lvrfrc87 merged commit 1631a6d into main Feb 10, 2022
@lvrfrc87 lvrfrc87 deleted the abstract_base_class branch February 10, 2022 08:18
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

Successfully merging this pull request may close these issues.

4 participants