chore: check not None path on datasamplespec #395
Conversation
Signed-off-by: ThibaultFy <[email protected]>
Signed-off-by: ThibaultFy <[email protected]>
There was a problem hiding this comment.
Thank for your work, some comments on typing ^^
Nitpick: If path or paths are expected to be existing dirs, we could use pydantic.DirectoryPath (https://docs.pydantic.dev/latest/api/types/#pydantic.types.DirectoryPath)
These changes may be need to be checked on Windows (but has we never cast to PosixPath, I expect it won't break anything)
substra/sdk/schemas.py
Outdated
| @@ -146,6 +146,30 @@ class DataSampleSpec(_Spec): | |||
| def is_many(self): | |||
| return self.paths and len(self.paths) > 0 | |||
|
|
|||
| @pydantic.field_validator("paths") | |||
| @classmethod | |||
| def resolve_paths(cls, v: typing.Any) -> typing.Any: | |||
There was a problem hiding this comment.
| def resolve_paths(cls, v: typing.Any) -> typing.Any: | |
| def resolve_paths(cls, v: Optional[List[pathlib.Path]]) -> Optional[List[pathlib.Path]]: |
There was a problem hiding this comment.
Oh great. And I removed the "optional" in that case, as, if the field is specified, this is not optional anymore to have a path. WDT ?
substra/sdk/schemas.py
Outdated
|
|
||
| @pydantic.field_validator("path") | ||
| @classmethod | ||
| def resolve_path(cls, v: typing.Any) -> typing.Any: |
There was a problem hiding this comment.
| def resolve_path(cls, v: typing.Any) -> typing.Any: | |
| def resolve_path(cls, v: Optional[pathlib.Path]) -> Optional[pathlib.Path]: |
substra/sdk/schemas.py
Outdated
|
|
||
| paths = [] | ||
| for path in v: | ||
| path = pathlib.Path(path) |
There was a problem hiding this comment.
| path = pathlib.Path(path) |
pydantic already casts str to path when using the type pathlib.Path
There was a problem hiding this comment.
Oh I didn't knew ! So nice...
substra/sdk/schemas.py
Outdated
| paths = [] | ||
| for path in v: | ||
| path = pathlib.Path(path) | ||
| paths.append(path.resolve()) | ||
|
|
||
| return paths |
There was a problem hiding this comment.
nitpick: style change, you can disregard
| paths = [] | |
| for path in v: | |
| path = pathlib.Path(path) | |
| paths.append(path.resolve()) | |
| return paths | |
| return [p.resolve() for p in paths] |
substra/sdk/schemas.py
Outdated
| if v is None: | ||
| raise ValueError("'path' cannot be set to None.") | ||
|
|
||
| path = pathlib.Path(v) |
There was a problem hiding this comment.
| path = pathlib.Path(v) |
Signed-off-by: ThibaultFy <[email protected]>
Related issue
#followed by the number of the issueSummary
Notes
Please check if the PR fulfills these requirements