Skip to content

Conversation

@AbyAbraham21
Copy link
Collaborator

Basic checks for data types passed into run.init() based on pydantic models defined in models.py

@AbyAbraham21 AbyAbraham21 requested a review from alahiff December 12, 2022 08:27
simvue/run.py Outdated
try:
runinput = RunInput(**data)
except ValidationError as e:
self._error(json.dumps(e.json()))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think using instead here:

            self._error(e)

rather than:

            self._error(json.dumps(e.json()))

makes things much easier to understand. Using json you get an error like this:

RuntimeError: "[\n  {\n    \"loc\": [\n      \"metadata\",\n      \"hello\"\n    ],\n    \"msg\": \"str type expected\",\n    \"type\": \"type_error.str\"\n  },\n  {\n    \"loc\": [\n      \"metadata\",\n      \"hello\"\n    ],\n    \"msg\": \"value is not a valid integer\",\n    \"type\": \"type_error.integer\"\n  },\n  {\n    \"loc\": [\n      \"metadata\",\n      \"hello\"\n    ],\n    \"msg\": \"value is not a valid float\",\n    \"type\": \"type_error.float\"\n  }\n]"

whereas with plain text you get:

RuntimeError: 3 validation errors for RunInput
metadata -> hello
  str type expected (type=type_error.str)
metadata -> hello
  value is not a valid integer (type=type_error.integer)
metadata -> hello
  value is not a valid float (type=type_error.float)

For me at least this is easier to understand.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, agreed. Will update.

@alahiff
Copy link
Collaborator

alahiff commented Dec 12, 2022

If possible we should also delete the lines:

        if not folder.startswith('/'):
            self._error('the folder must begin with /')

and update models.py to check for this instead. I noticed that if you specify something weird as the folder, e.g. a dict, the above gives an error (as expected).

@alahiff
Copy link
Collaborator

alahiff commented Dec 12, 2022

Updating models to have this seems to work:

    folder: constr(regex=r'^/.*')

@alahiff
Copy link
Collaborator

alahiff commented Dec 12, 2022

@alahiff alahiff merged commit 4120d22 into main Dec 12, 2022
@alahiff alahiff deleted the validation-checks-for-RunInput branch December 12, 2022 13:03
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.

3 participants