Skip to content

remove unused typevars, make dtype type alias #265

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 28, 2023

Conversation

MarcoGorelli
Copy link
Contributor

Summary:

@MarcoGorelli MarcoGorelli added the static typing type annotations, use of type checkers directly from the spec label Sep 27, 2023
@MarcoGorelli MarcoGorelli force-pushed the dtype-alias-remove-typevar branch from 2de69ca to 3a52ce8 Compare September 27, 2023 13:35
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @MarcoGorelli. This LGTM overall. In particular, going from dtype: Any to dtype: DType seems nice.

My one question is about removing support for parametrizing Column by its dtype (Column[Bool] et al). Do you see that as an improvement, or a (perhaps temporary) regression?

@MarcoGorelli
Copy link
Contributor Author

a teeny tiny regression perhaps, but I think the DType type alias would be more useful to devs, so I'd happily prioritise that one

in any case, if we did go ahead with #249 , then individual columns would become less relevant (and would probaby only be a last step before converting to ndarray anyway, or an initial step during construction)

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, SGTM! This affects only static typing and html docs, rather than an API change. So let me hit the green button here.

@rgommers rgommers merged commit fd32b6b into data-apis:main Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
static typing type annotations, use of type checkers directly from the spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants