Conversation
| # TODO: Move TableProperties.DEFAULT_FORMAT_VERSION to separate file and set that as format_version default. | ||
| self, | ||
| new_schema: Schema | "pa.Schema", | ||
| new_schema: Schema | pa.Schema, |
There was a problem hiding this comment.
I'm not sure if this is safe, since we have a guard to avoid circular dependencies:
if TYPE_CHECKING:
import pyarrow as pa
There was a problem hiding this comment.
shouldnt our linter catch this?
There was a problem hiding this comment.
I agree with this in theory. The fact that CI passes (and our linter) suggests that it might not be the case
There was a problem hiding this comment.
From my LLM:
Yes, it's safe to remove the quotes from "pa.Schema" and change it to just pa.Schema. Here's why:
- TYPE_CHECKING guard: Since pyarrow is imported within the TYPE_CHECKING block, it's only available during static type checking, not at runtime.
- PEP 563 compatibility: With from future import annotations at the top of the file (line 17), all annotations are automatically stringified at runtime anyway, so the quotes are redundant.
Makes sense to me 😄
There was a problem hiding this comment.
PEP 563 compatibility: With from future import annotations at the top of the file (line 17), all annotations are automatically stringified at runtime anyway, so the quotes are redundant.
Ah nice, thanks 👍
3 participants
Part of #2700
Rationale for this change
This enables linter rule UP037
Are these changes tested?
make lintandmake testshould pass.Are there any user-facing changes?