Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 72 additions & 0 deletions src/hdmf/validate/errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,78 @@ def __eq__(self, other):
return hash(self) == hash(other)


class ValidationWarning:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since ValidationWarning is almost entirely identical to Error, it would be better to create a shared base class ValidationIssue that both extend from and that contains all the shared code. Downstream code will still be able to differentiate between Error and ValidationWarning because they are different classes.

One minor thing is that because __eq__ is defined based on the hash and I believe the hash of a ValidationWarning and the hash of an identical Error object will be the same, then Error("name", "reason") == ValidationWarning("name", "reason") will be True. This might become an issue if/when errors and warnings are placed in a dict together.

To address this, I would change __eq__ to:

    def __eq__(self, other):
          return type(self) is type(other) and hash(self) == hash(other)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the below change to ValidationWarning.__eq__.

Since ValidationWarning and Error share significant code, could you also create a shared base class ValidationIssue that both extend from and that contains all the shared code? That would also make Error.__eq__ match ValidationWarning.__eq__, which it should.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

With these changes, could you also add a quick test that an error and warning with the same name are not equal, so that these changed lines are tested?


@docval({'name': 'name', 'type': str, 'doc': 'the name of the component that is erroneous'},
{'name': 'reason', 'type': str, 'doc': 'the reason for the warning'},
{'name': 'location', 'type': str, 'doc': 'the location of the warning', 'default': None})
def __init__(self, **kwargs):
self.__name = getargs('name', kwargs)
self.__reason = getargs('reason', kwargs)
self.__location = getargs('location', kwargs)

@property
def name(self):
return self.__name

@property
def reason(self):
return self.__reason

@property
def location(self):
return self.__location

@location.setter
def location(self, loc):
self.__location = loc

def __str__(self):
return self.__format_str(self.name, self.location, self.reason)

@staticmethod
def __format_str(name, location, reason):
if location is not None:
return "%s (%s): %s" % (name, location, reason)
else:
return "%s: %s" % (name, reason)

def __repr__(self):
return self.__str__()

def __hash__(self):
return hash(self.__equatable_str())

def __equatable_str(self):
if self.location is not None:
equatable_name = self.name.split('/')[-1]
else:
equatable_name = self.name
return self.__format_str(equatable_name, self.location, self.reason)

def __eq__(self, other):
return hash(self) == hash(other)


class ValidationResult:

def __init__(self, errors = None, warnings = None):
self.errors = list(errors) if errors is not None else []
self.warnings = list(warnings) if errors is not None else []
Comment thread
rly marked this conversation as resolved.
Outdated

def __iter__(self):
return iter(self.errors)

def __len__(self):
return len(self.errors)

def __bool__(self):
return bool(self.errors)

def __getitem__(self, i):
return self.errors[i]
Comment thread
rly marked this conversation as resolved.


class DtypeError(Error):

@docval({'name': 'name', 'type': str, 'doc': 'the name of the component that is erroneous'},
Expand Down
7 changes: 4 additions & 3 deletions src/hdmf/validate/validator.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import numpy as np

from .errors import Error, DtypeError, MissingError, MissingDataType, ShapeError, IllegalLinkError, IncorrectDataType
from .errors import ExpectedArrayError, IncorrectQuantityError
from .errors import ExpectedArrayError, IncorrectQuantityError, ValidationResult
from ..build import GroupBuilder, DatasetBuilder, LinkBuilder, ReferenceBuilder
from ..build.builders import BaseBuilder
from ..spec import Spec, AttributeSpec, GroupSpec, DatasetSpec, RefSpec, LinkSpec
Expand Down Expand Up @@ -325,7 +325,7 @@ def get_validator(self, **kwargs):
raise ValueError(msg)

@docval({'name': 'builder', 'type': BaseBuilder, 'doc': 'the builder to validate'},
returns="a list of errors found", rtype=list)
returns="a list of errors found", rtype=(list, ValidationResult))
Comment thread
rly marked this conversation as resolved.
Outdated
def validate(self, **kwargs):
"""Validate a builder against a Spec

Expand All @@ -338,7 +338,8 @@ def validate(self, **kwargs):
msg = "builder must have data type defined with attribute '%s'" % self.__type_key
raise ValueError(msg)
validator = self.get_validator(dt)
return validator.validate(builder)
errors_list = validator.validate(builder)
return ValidationResult(errors=errors_list, warnings=[])


class Validator(metaclass=ABCMeta):
Expand Down
Loading