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

chore: check not None path on datasamplespec #395

Merged
merged 3 commits into from
Feb 28, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 24 additions & 18 deletions substra/sdk/schemas.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def resolve_paths(cls, v: typing.Any) -> typing.Any:
def resolve_paths(cls, v: Optional[List[pathlib.Path]]) -> Optional[List[pathlib.Path]]:

Copy link
Member Author

Choose a reason for hiding this comment

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

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 ?

"""Resolve given paths."""
if v is None:
raise ValueError("'paths' cannot be set to None.")

paths = []
for path in v:
path = pathlib.Path(path)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
path = pathlib.Path(path)

pydantic already casts str to path when using the type pathlib.Path

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I didn't knew ! So nice...

paths.append(path.resolve())

return paths
Copy link
Contributor

@guilhem-barthes guilhem-barthes Nov 2, 2023

Choose a reason for hiding this comment

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

nitpick: style change, you can disregard

Suggested change
paths = []
for path in v:
path = pathlib.Path(path)
paths.append(path.resolve())
return paths
return [p.resolve() for p in paths]


@pydantic.field_validator("path")
@classmethod
def resolve_path(cls, v: typing.Any) -> typing.Any:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def resolve_path(cls, v: typing.Any) -> typing.Any:
def resolve_path(cls, v: Optional[pathlib.Path]) -> Optional[pathlib.Path]:

"""Resolve given path."""
if v is None:
raise ValueError("'path' cannot be set to None.")

path = pathlib.Path(v)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
path = pathlib.Path(v)

return path.resolve()

@pydantic.model_validator(mode="before")
@classmethod
def exclusive_paths(cls, values: typing.Any) -> typing.Any:
Expand All @@ -156,24 +180,6 @@ def exclusive_paths(cls, values: typing.Any) -> typing.Any:
raise ValueError("'path' or 'paths' field must be set.")
return values

@pydantic.model_validator(mode="before")
@classmethod
def resolve_paths(cls, values: typing.Any) -> typing.Any:
"""Resolve given path is relative."""
if "paths" in values:
paths = []
for path in values["paths"]:
path = pathlib.Path(path)
paths.append(path.resolve())

values["paths"] = paths

elif "path" in values:
path = pathlib.Path(values["path"])
values["path"] = path.resolve()

return values

@contextlib.contextmanager
def build_request_kwargs(self, local):
# redefine kwargs builder to handle the local paths
Expand Down
10 changes: 10 additions & 0 deletions tests/sdk/test_schemas.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,13 @@ def test_datasample_spec_exclusive_path():
def test_datasample_spec_no_path():
with pytest.raises(ValueError):
DataSampleSpec(data_manager_keys=[str(uuid.uuid4())])


def test_datasample_spec_paths_set_to_none():
with pytest.raises(ValueError):
DataSampleSpec(paths=None, data_manager_keys=[str(uuid.uuid4())])


def test_datasample_spec_path_set_to_none():
with pytest.raises(ValueError):
DataSampleSpec(path=None, data_manager_keys=[str(uuid.uuid4())])
Loading