Add Py version check for using eval_str flag of the inspect.signature() #5083
Conversation
jmao-denver
added
NoDocumentationNeeded
python-server-side
NoReleaseNotesNeeded
| """ Parse a specific type (top level or nested in a top-level Union annotation) without handling nested types | ||
| (e.g. a nested Union). The result is stored in the given _ParsedAnnotation object. | ||
| """ | ||
| t = eval(t) if isinstance(t, str) else t |
There was a problem hiding this comment.
Is there a reason the eval can't go in the "old" code block of _parse_signature below instead of the logic being split across two files?
Does this line deserve a comment if it stays here?
There was a problem hiding this comment.
"eval' the specific type hints here makes sense because this is where they are parsed. _parse_signature handles the whole signature, it would be quite cluttering to dive into individual typing there.
| if sys.version_info.major == 3 and sys.version_info.minor >= 10: | ||
| sig = inspect.signature(fn, eval_str=True) | ||
| else: | ||
| sig = inspect.signature(fn) |
There was a problem hiding this comment.
Does __future__ provide a way to port this in a cleaner way?
There was a problem hiding this comment.
Not really. I think having an explicit version check actually helps future code cleanup when we bump the minimum supported Python version.
|
@chipkent / @jmao-denver can this PR get some attention/merged please? This is holding up some merges on our deephaven-plugins side, as it's causing our tests to break: deephaven/deephaven-plugins#217 |
Fixes #5075