-
Notifications
You must be signed in to change notification settings - Fork 22
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
[Test] Test Alessio's fix to schema generation #671
Conversation
self, file_paths: list[str | Path], verbose: bool = False | ||
self, file_paths: list, verbose: bool = False |
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.
Supporting the ability for people to call from __future__ import annotations
in the library is one thing; adding full support for schema inference from data interface signatures of the updated kind is another, and would in preferance be done with Pydantic
Removing for now
for more information, see https://pre-commit.ci
src/neuroconv/utils/json_schema.py
Outdated
if isinstance(arg, str): | ||
arg = type(arg) |
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.
Well, apparently all the new types under this behavior become str
and so everything gets mapped to string
in the JSON type map...
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.
Why not we just use https://github.com/shawwn/get-annotations for python < 3.10
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.
Let's try it!
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.
Despite the input argument eval_str
saying it handles exactly this case, it doesn't look like it actually does what is says anywhere, and ends up at https://github.com/shawwn/get-annotations/blob/main/get_annotations/__init__.py#L86 regardless
@@ -130,6 +131,9 @@ def get_schema_from_method_signature(method: Callable, exclude: list = None) -> | |||
input_schema["properties"].update({param_name: dict(format="directory")}) | |||
else: | |||
arg = param.annotation | |||
# in case __future__.annotations is imported, the annotation is a string | |||
if isinstance(arg, str): | |||
arg = eval(arg) |
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.
Though note this could cause security risks. Should check and limit its application to the set of basic keys supported in the json mapping dictionary, if this works at all
Yeah IDK my efforts here can resolve the issues with common source data patterns, but the same logic is applied to conversion options and there are some cases that cause more and other issues after that I think we might just want to fix this by offloading all the effort required to Pydantic as in re-implementing #264 |
replaced by #1016 |
No description provided.