Skip to content
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

Replace with_row_count by with_row_index for Polars #189

Merged
merged 5 commits into from
Feb 12, 2024

Conversation

jrycw
Copy link
Contributor

@jrycw jrycw commented Feb 10, 2024

As with_row_count is deprecated, it's recommended to use with_row_index instead.

@machow
Copy link
Collaborator

machow commented Feb 12, 2024

Thanks for submitting this! I wonder if we need a guard for backward compatibility with earlier polars versions? WDYT of using something like f = getattr(data._tbl_data, "with_row_index", None), and then if the method isn't there, using with_row_count()?

@jrycw
Copy link
Contributor Author

jrycw commented Feb 12, 2024

Thanks for submitting this! I wonder if we need a guard for backward compatibility with earlier polars versions? WDYT of using something like f = getattr(data._tbl_data, "with_row_index", None), and then if the method isn't there, using with_row_count()?

It seems that the developers of Polars are diligently working towards releasing version v1.0 in the near future. Considering this, we may no longer need to maintain backward compatibility for earlier versions. As with_row_index is expected to be the stable API for Polars v1.0, we can proceed with this assumption.

@machow
Copy link
Collaborator

machow commented Feb 12, 2024

Thanks for weighing in--it looks like with_row_index was added within the last month. We need to support versions of polars from more than a month ago, just in case. If it's okay with you, I'm happy to make the change and then merge.

Thanks again for submitting the PR, it's really helpful to see this deprecation in polars, and get ahead of it

@jrycw
Copy link
Contributor Author

jrycw commented Feb 12, 2024

Thanks for weighing in--it looks like with_row_index was added within the last month. We need to support versions of polars from more than a month ago, just in case. If it's okay with you, I'm happy to make the change and then merge.

Thanks again for submitting the PR, it's really helpful to see this deprecation in polars, and get ahead of it

Sounds good to me. Please proceed.

@jrycw
Copy link
Contributor Author

jrycw commented Feb 12, 2024

Thanks for submitting this! I wonder if we need a guard for backward compatibility with earlier polars versions? WDYT of using something like f = getattr(data._tbl_data, "with_row_index", None), and then if the method isn't there, using with_row_count()?

Perhaps using f = getattr(data._tbl_data, "with_row_index", "with_row_count") would be neater.

@rich-iannone rich-iannone self-requested a review February 12, 2024 20:34
@machow
Copy link
Collaborator

machow commented Feb 12, 2024

@rich-iannone just a heads up, I did a tiny bit more CI tweaking to get the docs job to skip trying to push to netlify and deployment steps, when running on a fork!

@machow machow merged commit cc19ed7 into posit-dev:main Feb 12, 2024
7 checks passed
@rich-iannone
Copy link
Member

@rich-iannone just a heads up, I did a tiny bit more CI tweaking to get the docs job to skip trying to push to netlify and deployment steps, when running on a fork!

Thanks for doing that! Should help a lot for community PRs that focus on improving the docs.

@jrycw
Copy link
Contributor Author

jrycw commented Feb 12, 2024

Thanks for submitting this! I wonder if we need a guard for backward compatibility with earlier polars versions? WDYT of using something like f = getattr(data._tbl_data, "with_row_index", None), and then if the method isn't there, using with_row_count()?

Perhaps using f = getattr(data._tbl_data, "with_row_index", "with_row_count") would be neater.

The updated code looks good. However, I just realized that what I actually meant was something along the lines of f = getattr(data, "with_row_index", data.with_row_count) or f = getattr(data, "with_row_index", getattr(data, "with_row_count")).

@jrycw jrycw deleted the update-polars branch February 13, 2024 00:04
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.

None yet

3 participants