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

Can pass a JSON schema to DictParameter and ListParameter #3217

Merged
merged 7 commits into from
Jan 16, 2023

Conversation

adrien-berchet
Copy link
Contributor

@adrien-berchet adrien-berchet commented Jan 12, 2023

Description

Add an optional parameter to DictParameter and ListParameter so the loaded value can be validated against a JSON schema.

Motivation and Context

Adding a simple validation step reduces the amount of code in the run method of the tasks. Also, the arguments are checked at the beginning of the workflow so it fails faster, which is always better than failing during the workflow.

Have you tested this? If so, how?

I added simple tests.

@adrien-berchet
Copy link
Contributor Author

Note that this PR adds the jsonschema dependency. If you prefer I can make it optional.

@adrien-berchet adrien-berchet changed the title Can pass a JSON schema to DictParameter Can pass a JSON schema to DictParameter and ListParameter Jan 13, 2023
Copy link
Collaborator

@dlstadther dlstadther left a comment

Choose a reason for hiding this comment

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

This is a great addition!

However, I'm leaning towards suggesting jsonschema be an optional installation since this is an optional feature usage.

@adrien-berchet
Copy link
Contributor Author

Cool, thanks!
Ok, I pushed a new commit to make it optional. Now a warning is raised is the user tries to use the new schema parameter while jsonschema is not installed.

Copy link
Collaborator

@dlstadther dlstadther left a comment

Choose a reason for hiding this comment

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

Thanks!

@dlstadther dlstadther merged commit b6ff4c0 into spotify:master Jan 16, 2023
@adrien-berchet adrien-berchet deleted the dict_schema branch January 16, 2023 15:56
@adrien-berchet
Copy link
Contributor Author

With pleasure :)

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.

2 participants