-
Notifications
You must be signed in to change notification settings - Fork 1
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
Improve type hinting and favor statically typed parameters for Modality and Platform #6
Improve type hinting and favor statically typed parameters for Modality and Platform #6
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bruno-f-cruz Take a look at my comments. From what I understand you cannot type a platform (or modality) attribute as a Platform model type.
@@ -44,7 +44,7 @@ class ManifestConfig(BaseModel): | |||
description="where to send data to on VAST", | |||
title="VAST destination and maybe S3?", | |||
) | |||
modalities: Dict[str, List[str]] = Field( | |||
modalities: Dict[Modality, List[str]] = Field( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change Modality to Modality.ONE_OF. This should allow users to input a string that the Modality model can validate
@@ -30,11 +30,11 @@ class ManifestConfig(BaseModel): | |||
description="Transfer time to schedule copy and upload. If None defaults to trigger the transfer immediately", # noqa | |||
title="APScheduler transfer time", | |||
) | |||
platform: str = Field(description="Platform type", title="Platform type") | |||
platform: Platform = Field(description="Platform type", title="Platform type") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change Platform to Platform.ONE_OF. This should allow users to input a string that the Platform model can validate
Relevant to this problem: AllenNeuralDynamics/aind-data-schema#960 I think we should code our own wrapper around the aind-data-schema for now that uses the same syntax class Modality(Enum, str):
"""Modality classes"""
BEHAVIOR = modaloties.Modality.Behavior().name
BEHAVIOR_VIDEOS = modaloties.Modality.BehaviorVideos().name This solves a few problems:
It has a few cons: |
I will add this wrapper to another issue. Does that work? |
This PR refactors the
ManifestConfig
class by improving the type hinting for some of the fields.This allows easier validation (without the need for custom validators) as well as auto-completion.