Blocks core typing #16218
Blocks core typing #16218
Conversation
CodSpeed Performance ReportMerging #16218 will not alter performanceComparing Summary
|
zzstoatzz
force-pushed
the
blocks-core-typing
branch
from
e359003 to
fec6122
Compare
zzstoatzz
force-pushed
the
blocks-core-typing
branch
from
fec6122 to
e70ac6f
Compare
src/prefect/blocks/core.py
Outdated
| block_type=BlockTypeUpdate.model_construct( | ||
| **local_block_type.model_dump() | ||
| ), |
There was a problem hiding this comment.
It might be worthwhile to add a to_block_type_update to BlockType to avoid a model_dump.
| @@ -145,7 +149,7 @@ def _collect_secret_fields( | |||
| for nested_type in get_args(type_): | |||
| _collect_secret_fields(name, nested_type, secrets) | |||
| return | |||
| elif _is_subclass(type_, BaseModel): | |||
| elif _is_subclass(type_, BaseModel) and type_ is not None: | |||
There was a problem hiding this comment.
I'm surprised this is needed. None is never a subclass of BaseModel and I would expect a type checker to also understand this.
| @@ -160,7 +164,7 @@ def _collect_secret_fields( | |||
| # Append .* to field name to signify that all values under this | |||
| # field are secret and should be obfuscated. | |||
| secrets.append(f"{name}.*") | |||
| elif Block.is_block_class(type_): | |||
| elif Block.is_block_class(type_) and type_ is not None: | |||
There was a problem hiding this comment.
If you make Block.is_block_class a type guard you won't need the extra check here.
| """Walk through the annotation and collect block schemas for any nested blocks.""" | ||
| if Block.is_block_class(annotation): | ||
| if Block.is_block_class(annotation) and annotation is not None: |
There was a problem hiding this comment.
Same here, Block.is_block_class() is a prime candidate for conversion to a type guard.
| try: | ||
| block_document = await client.read_block_document_by_name( | ||
| block_document = await cast( |
There was a problem hiding this comment.
I prefer to use asserts over casts, because asserts can be had for 0 runtime cost when you use a if TYPE_CHECKING: guard.
I presume that client is Optional[] here only because type annotating the keyword argument in a decorator signature is hard?
If so then i think the decorator can be updated with protocols to capture this much better.
There was a problem hiding this comment.
🤦 I forgot that you can't use type annotations to change the type of a keyword argument with decorators. The current type annotations is the best you can hope for.
I'd still use if TYPE_CHECKING: assert client is not None instead of cast() because the TYPE_CHECKING guard is zero-cost whereas cast() still takes bytecode cycles for the name lookup and call.
i wish I didn't have to use
constructhere, maybe there's a better way? @desertaxle