Skip to content

Overload bids2table signature for specific return types#39

Closed
shnizzedy wants to merge 2 commits into
childmindresearch:mainfrom
shnizzedy:overload/bids2table
Closed

Overload bids2table signature for specific return types#39
shnizzedy wants to merge 2 commits into
childmindresearch:mainfrom
shnizzedy:overload/bids2table

Conversation

@shnizzedy
Copy link
Copy Markdown

Instead of typehinting BIDSTable | None for the return type of bids2table, this PR updates the typehinting to dynamically be BIDSTable for bids2table(return_table=True) (or unspecified, which with return_table=True by default) or None for bids2table(return_table=False).

screen-capture.webm

The functionality is the same between just a4bcf5e or both commits here. The difference is the first commit does the overload in the main file

@overload
def bids2table(
root: StrOrPath,
*,
with_meta: bool = True,
persistent: bool = False,
index_path: Optional[StrOrPath] = None,
exclude: Optional[List[str]] = None,
incremental: bool = False,
overwrite: bool = False,
workers: Optional[int] = None,
worker_id: Optional[int] = None,
return_table: Literal[True] = True,
) -> BIDSTable: ...
@overload
def bids2table(
root: StrOrPath,
*,
with_meta: bool = True,
persistent: bool = False,
index_path: Optional[StrOrPath] = None,
exclude: Optional[List[str]] = None,
incremental: bool = False,
overwrite: bool = False,
workers: Optional[int] = None,
worker_id: Optional[int] = None,
return_table: Literal[False],
) -> None: ...
def bids2table(
root: StrOrPath,
*,
with_meta: bool = True,
persistent: bool = False,
index_path: Optional[StrOrPath] = None,
exclude: Optional[List[str]] = None,
incremental: bool = False,
overwrite: bool = False,
workers: Optional[int] = None,
worker_id: Optional[int] = None,
return_table: bool = True,
) -> Optional[BIDSTable]:
while the second commit moves the overload to a stub file for the sake of abstraction / less scrolling in the main file.

No hard feelings if you don't want this specific typehinting.

@clane9
Copy link
Copy Markdown
Contributor

clane9 commented May 6, 2025

Thanks for this PR @shnizzedy! I agree it is a bit awkward for the return value to be this union BIDSTable | None. In #48, I decided to simplify the API so that the new indexing function index_dataset always just returns a pyarrow table. Saving as a parquet (or any other format) then is just a single pyarrow call. E.g.

import pyarrow.parquet as pq
import bids2table as b2t2

tab = b2t2.index_dataset("s3://openneuro.org/ds000102")
pq.write_table(tab, "ds000102.parquet")

That said, I'd be very open to any feedback on the new API. If you have any issues or suggestions, hope you'll open a new issue 🙏.

@clane9 clane9 closed this May 6, 2025
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.

2 participants