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! |
sydney-runkle
left a comment
There was a problem hiding this comment.
Hey @nix010,
Great start here. Left some general feedback + a few test requests!
sydney-runkle
left a comment
There was a problem hiding this comment.
Some ideas for simplification :)
| import pathlib | ||
|
|
||
| if source_type not in { | ||
| orig_source_type = get_origin(source_type) or source_type |
There was a problem hiding this comment.
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.
@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.
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.
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.
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.
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.
Hmm, I wonder if we want to change the JSON schema for these path types...
There was a problem hiding this comment.
sure, what should we change them into ?
There was a problem hiding this comment.
I feel like the type should be bytes for the byte_type path...
There was a problem hiding this comment.
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.
Bc it's really a string once you populate the path
Change Summary
Fix #9706
Related issue number
Fix #9706
Checklist
Selected Reviewer: @hramezani