fix: refine type annotation#3278
Conversation
| yield LanceFragment(dataset, int(f)) | ||
| elif isinstance(f, FragmentMetadata): | ||
| yield LanceFragment(dataset, f.fragment_id) | ||
| yield LanceFragment(dataset, f.id) |
There was a problem hiding this comment.
I changed fragment_id to id. because fragment metadata only has attribute id
|
maybe nextstep we can use pyright to guarantee python code quality |
westonpace
left a comment
westonpace
left a comment
There was a problem hiding this comment.
This is quite thorough. Did you use a tool of any kind to make it?
Overall I think these changes are good but might check @wjones127 for a second opinion.
| If specified, use the provided commit handler to lock the table while | ||
| committing a new version. Not necessary on object stores other than S3 | ||
| or when there are no concurrent writers. |
There was a problem hiding this comment.
| If specified, use the provided commit handler to lock the table while | |
| committing a new version. Not necessary on object stores other than S3 | |
| or when there are no concurrent writers. | |
| A custom commit lock. Only needed if your object store does not support | |
| atomic commits. See the user guide for more details. |
| class Tag(TypedDict): | ||
| version: int | ||
| manifest_size: int | ||
|
|
||
|
|
||
| class Version(TypedDict): | ||
| version: int | ||
| timestamp: int | datetime | ||
| metadata: Dict[str, str] | ||
|
|
||
|
|
||
| class UpdateResult(TypedDict): | ||
| num_rows_updated: int | ||
|
|
||
|
|
||
| class AlterColumn(TypedDict): | ||
| path: str | ||
| name: Optional[str] | ||
| nullable: Optional[bool] | ||
| data_type: Optional[pa.DataType] | ||
|
|
||
|
|
||
| class ExecuteResult(TypedDict): | ||
| num_inserted_rows: int | ||
| num_updated_rows: int | ||
| num_deleted_rows: int | ||
|
|
||
|
|
||
| class Index(TypedDict): | ||
| name: str | ||
| type: str | ||
| uuid: str | ||
| fields: List[str] | ||
| version: int | ||
| fragment_ids: Set[int] | ||
|
|
||
|
|
||
| class OperationDict(TypedDict): | ||
| fragments: List[_FragmentMetadata] |
There was a problem hiding this comment.
It might be nice to document these properties but I think most of them should be obvious and that can always be done in a future PR.
| def __lt__(self, other: BFloat16) -> bool: ... | ||
| def __le__(self, other: BFloat16) -> bool: ... | ||
| def __eq__(self, other: object) -> bool: ... | ||
| def __ne__(self, other: object) -> bool: ... | ||
| def __gt__(self, other: BFloat16) -> bool: ... | ||
| def __ge__(self, other: BFloat16) -> bool: ... |
There was a problem hiding this comment.
These methods seem out of place for _DataFile.
| ): ... | ||
| def _json_to_schema(schema_json: str) -> pa.Schema: ... | ||
| def _schema_to_json(schema: pa.Schema) -> str: ... | ||
| def trace_to_chrome(file: Optional[str] = None, level: Optional[str] = None): ... |
There was a problem hiding this comment.
Shouldn't this be in the tracing package?
There was a problem hiding this comment.
do you mean it should be organized like optimize.pyi, debug.pyi? But I get atttribute access issue, when I use pyright to check these files. putting all definition in init.pyi can solve this issue, because actually there's only one dynlib.
There was a problem hiding this comment.
I reorg the file path, now pyright doesn't complain error
| class UpdateResult(TypedDict): | ||
| num_rows_updated: int | ||
|
|
||
|
|
||
| class AlterColumn(TypedDict): | ||
| path: str | ||
| name: Optional[str] | ||
| nullable: Optional[bool] | ||
| data_type: Optional[pa.DataType] | ||
|
|
||
|
|
||
| class ExecuteResult(TypedDict): | ||
| num_inserted_rows: int | ||
| num_updated_rows: int | ||
| num_deleted_rows: int |
There was a problem hiding this comment.
Some of these (e.g. UpdateResult and ExecuteResult) we had as a typed dict because we thought it might be likely we would add properties in the future and wanted to avoid this being a breaking change.
I wonder if this still holds if we have types declared. I guess, if this makes it a breaking change then it was technically always a breaking change so maybe it isn't a big deal 😆
There was a problem hiding this comment.
why will it be a break change? in the runtime, typeddict is just dict.
There was a problem hiding this comment.
Good point. I guess I'm not really sure why we used typed dict then 😕. Either way, I'm happy with the change.
3 participants
No description provided.