GH1033 Add overloads of engine for pd.read_json #1035
Conversation
|
For |
loicdiridollou
changed the title
There was a problem hiding this comment.
I'm a little concerned about the misuse of ellipses with default arguments. Ellipses should only be used in an argument when the argument is optional. When you want a specific result to happen as a result of the argument being specified, you don't use ellipses. The overloads that require values to be specified (i.e., the ones without ellipses) should come before the ones that use ellipses. And the ones with ellipses should have "broad" types. So writing something like engine: Literal["pyarrow"] = ... can't be correct, because the default value of engine is ujson, so if the stub is to work without specification of that parameter it would be engine: Literal["ujson", "pyarrow"] = ... .
| lines: Literal[True] = ..., | ||
| chunksize: None = ..., | ||
| compression: CompressionOptions = ..., | ||
| nrows: int | None = ..., | ||
| storage_options: StorageOptions = ..., | ||
| dtype_backend: DtypeBackend | NoDefault = ..., | ||
| engine: Literal["pyarrow"] = ..., |
There was a problem hiding this comment.
Since the default value of lines is False, if you have ... for both lines and engine, it means that you don't have to specify either. So I think you don't want the ellipses here on either argument.
| @@ -72,6 +98,7 @@ def read_json( | |||
| nrows: int | None = ..., | |||
| storage_options: StorageOptions = ..., | |||
| dtype_backend: DtypeBackend | NoDefault = ..., | |||
| engine: Literal["pyarrow"] = ..., | |||
There was a problem hiding this comment.
I think the ellipses here should be removed.
| lines: Literal[True], | ||
| chunksize: int, | ||
| compression: CompressionOptions = ..., | ||
| nrows: int | None = ..., | ||
| storage_options: StorageOptions = ..., | ||
| dtype_backend: DtypeBackend | NoDefault = ..., | ||
| engine: Literal["pyarrow"] = ..., |
| lines: Literal[True] = ..., | ||
| chunksize: None = ..., | ||
| compression: CompressionOptions = ..., | ||
| nrows: int | None = ..., | ||
| storage_options: StorageOptions = ..., | ||
| dtype_backend: DtypeBackend | NoDefault = ..., | ||
| engine: Literal["pyarrow"] = ..., |
There was a problem hiding this comment.
same comment about the ellipses
| check( | ||
| assert_type( | ||
| pd.read_json(dd, lines=True, engine="pyarrow"), | ||
| pd.DataFrame, | ||
| ), | ||
| pd.DataFrame, | ||
| ) |
There was a problem hiding this comment.
You should add a test with TYPE_CHECKING_INVALID_USAGE that makes sure that we disallow lines=False with engine="pyarrow" .
assert_type()to assert the type of any return value