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

Supports delayed evaluation of annotations #92

Merged
merged 2 commits into from
Mar 4, 2023
Merged

Supports delayed evaluation of annotations #92

merged 2 commits into from
Mar 4, 2023

Conversation

elijahbenizzy
Copy link
Collaborator

[Short description explaining the high-level reason for the pull request]

Changes

How I tested this

Notes

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future TODOs are captured in comments
  • Project documentation has been updated if adding/changing functionality.

@skrawcz
Copy link
Collaborator

skrawcz commented Mar 2, 2023

Just need to add a good commit message, and ensure graph adapters, etc., also get the requisite change.

@elijahbenizzy elijahbenizzy force-pushed the issue_91 branch 2 times, most recently from 1678405 to 2593107 Compare March 4, 2023 19:57
Luckily the code we have is fairly clean -- this doesn't have a big
blast radius. In reality, it should be one spot we make the changes, but
we'll have to refactor this later.

With __future__ import annotations, python now allows you to lazily
evaluate annotations. In <3.10, there is no way to get an
inspect.signature with fully evaluated annotations, so we have to use
typing.get_type_hints. In 3.10, there's a parameter to force-evaluate
signatures, so we can switch to use that when we deprecate 3.9 in quite
a while.

This is tested in the smoke-screen module -- testing is tricky due to
complexity of "future statements". We have to hack up imports, so I
chose only to do that in one place.
@elijahbenizzy
Copy link
Collaborator Author

Just need to add a good commit message, and ensure graph adapters, etc., also get the requisite change.

Yep, works with graph adapters, because we're relying on the produced nodes not the functions -- the abstraction made this generally fairly easy.

@elijahbenizzy elijahbenizzy changed the title Quick fix for #91 Supports delayed evaluation of annotations Mar 4, 2023
Currently there's just a note about postponed evaluation of annotations,
but we can add more here over time. This is meant to be specific API
reference stuff that doesn't really fit elsewhere.
@elijahbenizzy elijahbenizzy merged commit 5ff1885 into main Mar 4, 2023
@elijahbenizzy elijahbenizzy deleted the issue_91 branch March 4, 2023 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants