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

Skeleton.interBoneValidations should be replaced by something useful #1207

Open
phorward opened this issue Jul 9, 2024 · 1 comment
Open
Assignees
Labels
Priority: Medium This issue may be useful, and needs some attention. proposal refactoring Pull requests that refactor code but do not change its behavior. usability

Comments

@phorward
Copy link
Member

phorward commented Jul 9, 2024

interBoneValidations is one of the ugliest pieces in ViUR. It was intended to be used in the Skeleton to run validations on skeleton-level.

Levels of ugliness:

  1. The name interBoneValidations is ugly and misleading
  2. Why is it a list of validators, when all tests can be done in one function?
  3. Why does each function either return None or a list of ReadFromClientError?

Everywhere I've being used it, it looked liked this:

def _my_ugly_validation(skel):
    if skel["x"] in ("a", "c") and skel["y"] in ("a", "b"):
        return [
            skeleton.ReadFromClientError(
                skeleton.ReadFromClientErrorSeverity.Invalid,
                "No, but yes",
                ["x"]
            )
        ]
   

class MyUglySkeleton(Skeleton):
    interBoneValidations = [
        _my_ugly_validation
    ]

So in the end, it can be just a subclass-able hook function, let's call it just validate, which does the same, and belongs to the Skeleton class.

Proposal:

class MyLesserUglySkeleton(Skeleton):
    def validate(skel: SkeletonInstance) -> None | ReadFromClientError | t.Iterable[ReadFromClientError]:
        if skel["x"] in ("a", "c") and skel["y"] in ("a", "b"):
            return [
                skeleton.ReadFromClientError(
                    skeleton.ReadFromClientErrorSeverity.Invalid,
                    "No, but yes",
                    ["x"]
                )
            ]

This relates to #630 as well, which defines a well named and clear CRUD naming convention.

@phorward phorward added proposal Priority: Medium This issue may be useful, and needs some attention. refactoring Pull requests that refactor code but do not change its behavior. usability labels Jul 9, 2024
@phorward phorward self-assigned this Aug 9, 2024
@phorward
Copy link
Member Author

#1260 is a dynamic validation approach relating to this, too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Medium This issue may be useful, and needs some attention. proposal refactoring Pull requests that refactor code but do not change its behavior. usability
Projects
None yet
Development

No branches or pull requests

1 participant