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

[typing] prefect.utilities #16298

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

mjpieters
Copy link
Contributor

No description provided.

Copy link

codspeed-hq bot commented Dec 9, 2024

CodSpeed Performance Report

Merging #16298 will not alter performance

Comparing mjpieters:prefect_utilities_typing (bc236a1) with main (37bf294)

Summary

✅ 3 untouched benchmarks

@mjpieters mjpieters force-pushed the prefect_utilities_typing branch 5 times, most recently from 6734b73 to 731d01e Compare December 9, 2024 23:23
@@ -123,68 +123,32 @@ def create_task(coroutine: Coroutine) -> asyncio.Task:
return task


def _run_sync_in_new_thread(coroutine: Coroutine[Any, Any, T]) -> T:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just noting that this is unused, so removing it makes sense to me 👍

@mjpieters
Copy link
Contributor Author

@chrisguidry The following lines are (rightfully) marked by pyright as having an issue because obj["variable_name"] can be a dictionary here, as evident by the earlier checks:

@handler("workspace_variable")
def workspace_variable_handler(obj: dict[str, Any], ctx: HydrationContext) -> Any:
    if "variable_name" in obj:
        if isinstance(obj["variable_name"], dict):
            dehydrated_variable = _hydrate(obj["variable_name"], ctx)
        else:
            dehydrated_variable = obj["variable_name"]

        # If the result is a Placeholder, we should return it as is
        if isinstance(dehydrated_variable, Placeholder):
            return dehydrated_variable

        if not ctx.render_workspace_variables:
            return WorkspaceVariable(variable_name=obj["variable_name"])

Is it safe to assert that obj["variable_name"] is a str at this point or should this perhaps use dehydrated_variable instead (but asserting that dehydrated_variable is indeed a str)?

@chrisguidry
Copy link
Collaborator

Great question @mjpieters: I would use the dehydrated_variable there because it should be reduced down to the value you're looking for by then after maybe going through the _hydrate function, which could be rendering templates, etc.

@mjpieters mjpieters force-pushed the prefect_utilities_typing branch 7 times, most recently from a3a5ec3 to 9b27ace Compare December 10, 2024 18:37
@mjpieters mjpieters marked this pull request as ready for review December 10, 2024 18:40
@mjpieters mjpieters requested a review from cicdw as a code owner December 10, 2024 18:40
@mjpieters mjpieters force-pushed the prefect_utilities_typing branch 2 times, most recently from fc106ad to 94f853a Compare December 10, 2024 18:49
@mjpieters
Copy link
Contributor Author

This PR brings the type completeness up by 2.74%.

@mjpieters mjpieters force-pushed the prefect_utilities_typing branch from 94f853a to dd9f58e Compare December 10, 2024 18:55
Copy link
Collaborator

@zzstoatzz zzstoatzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! ill hold for @desertaxle to give a review as well

@mjpieters mjpieters force-pushed the prefect_utilities_typing branch from dd9f58e to bc236a1 Compare December 10, 2024 19:35
@zzstoatzz zzstoatzz merged commit e569cc5 into PrefectHQ:main Dec 10, 2024
38 checks passed
@mjpieters mjpieters deleted the prefect_utilities_typing branch December 10, 2024 21:51
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.

4 participants