-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Fix #9706 PathLike with subtype #9764
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
Fix #9706 PathLike with subtype #9764
Conversation
|
please review |
CodSpeed Performance ReportMerging #9764 will not alter performanceComparing Summary
|
|
Not sure if the change is align wiht what is expected from #9706 but made PR anyway to see if it on the right direction! |
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.
Hey @nix010,
Great start here. Left some general feedback + a few test requests!
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.
Some ideas for simplification :)
| @@ -207,7 +207,11 @@ def path_schema_prepare_pydantic_annotations( | |||
| ) -> tuple[Any, list[Any]] | None: | |||
| import pathlib | |||
|
|
|||
| if source_type not in { | |||
| orig_source_type = get_origin(source_type) or source_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.
What about:
orig_source_type: Any = get_origin(source_type) or source_type
if (source_type_args := get_args(source_type)) and source_type_args[0] not in {str, bytes, Any}:
return None
if orig_source_type not in {
os.PathLike,
pathlib.Path,
pathlib.PurePath,
pathlib.PosixPath,
pathlib.PurePosixPath,
pathlib.PureWindowsPath,
}:
return NoneThere 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.
@sydney-runkle so there's a test case like test_string_import_callable which make source_type_args[0] a list and result in TypeError: unhashable type: 'list'.
So I just check it below.
is_path_like_subtype_invalid = (
orig_source_type == os.PathLike
and source_type_args
and source_type_args[0]
not in {
str,
bytes,
Any,
}
)
if is_path_like_subtype_invalid:
return None 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.
I believe my suggestion above does the same...
| _known_annotated_metadata.check_metadata(metadata, _known_annotated_metadata.STR_CONSTRAINTS, source_type) | ||
| _known_annotated_metadata.check_metadata(metadata, _known_annotated_metadata.STR_CONSTRAINTS, orig_source_type) | ||
|
|
||
| construct_path = orig_source_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.
What about:
construct_path = pathlib.PurePath if orig_source_type is os.PathLike else orig_source_type
constrained_schema = (
core_schema.bytes_schema(**metadata) if source_type_args and source_type_args[0] is bytes
else core_schema.str_schema(**metadata)
)
def path_validator(input_value: str) -> os.PathLike[Any]:
try:
return construct_path(input_value)
except TypeError as e:
raise PydanticCustomError('path_type', 'Input is not a valid path') from eThere 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.
I realize I've skipped the decoding here, one moment... trying to add back in with correct typing
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.
I think this is type safe:
def path_validator(input_value: str | bytes) -> os.PathLike[str]:
try:
if source_type_args and source_type_args[0] is bytes:
if isinstance(input_value, bytes):
try:
input_value = input_value.decode()
except UnicodeDecodeError as e:
raise PydanticCustomError('bytes_type', 'Input must be valid bytes') from e
else:
raise PydanticCustomError('bytes_type', 'Input must be bytes')
elif not isinstance(input_value, str):
raise PydanticCustomError('path_type', 'Input is not a valid path')
return construct_path(input_value)
except TypeError as e:
raise PydanticCustomError('path_type', 'Input is not a valid path') from e| assert Model.model_json_schema() == { | ||
| 'properties': { | ||
| 'str_type': {'format': 'path', 'title': 'Str Type', 'type': 'string'}, | ||
| 'byte_type': {'format': 'path', 'title': 'Byte Type', 'type': 'string'}, |
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.
Hmm, I wonder if we want to change the JSON schema for these path types...
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.
sure, what should we change them into ?
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.
I feel like the type should be bytes for the byte_type path...
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.
Honestly though, if someone complains we can fix this, I don't think it's super high priority and this gives us some flexibility to change it based on user experience.
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.
Bc it's really a string once you populate the path
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.
Great, thanks!!
Change Summary
Fix #9706
Related issue number
Fix #9706
Checklist
Selected Reviewer: @hramezani