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

refactor!: temporary deprecate positional arguments of pl$Series() #966

Merged
merged 9 commits into from
Mar 26, 2024

Conversation

eitsupi
Copy link
Collaborator

@eitsupi eitsupi commented Mar 25, 2024

Part of #903

As of version 0.16, positional arguments are deprecated and a warning is occurred.

Copy link
Collaborator

@etiennebacher etiennebacher left a comment

Choose a reason for hiding this comment

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

I'd like to clarify a bit when this warning appears and when it doesn't before reviewing the rest

R/series__series.R Outdated Show resolved Hide resolved
R/series__series.R Outdated Show resolved Hide resolved
R/series__series.R Show resolved Hide resolved
@eitsupi eitsupi marked this pull request as ready for review March 25, 2024 15:00
Copy link
Collaborator

@etiennebacher etiennebacher left a comment

Choose a reason for hiding this comment

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

Thanks, some comments. I'm also wondering if we should made the change in argument position in 0.18.0 since we mention in #903 that

This would be a very huge breaking change, so we will probably have to continue warnings for several months and then make the change.

Given the rapid pace of development, I think 0.18.0 would be released in a couple of months. What I have in mind is that it's possible rust-polars 0.39.0 (and therefore r-polars 0.16.0) are released in a week or so, and 0.17.0 around the end of April. What do you think about keeping this deprecation warning for 2 full releases?

NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
@eitsupi
Copy link
Collaborator Author

eitsupi commented Mar 26, 2024

Thanks for your review.

What do you think about keeping this deprecation warning for 2 full releases?

This is a difficult question.

My concern is that the mechanism to fall back to the positional arguments is complicated, and I think it would be less confusing to remove this logic quickly.
That is, ideally pl$Series(name = "foo", "bar") should work fine in 0.15 and future versions, but this version will warning for this.
(Since there are originally three positional arguments, it is quite cumbersome to absorb all the patterns of positional and named arguments.)

So I think it would be better to replace the positional argument in 0.17 instead of 0.18 and remove this logic early to avoid the warning.

Also, as like I have substituted tests here, it should be easy to fix even with this breaking change, since in most cases it should be possible to fix it by simply substituting pl$Series( for as_polars_series(.

NEWS.md Show resolved Hide resolved
@etiennebacher
Copy link
Collaborator

What I'm worried about is that we have no control over releases in rust-polars, and the time span between 0.39.0 and 0.40.0 will correspond to the time of our deprecation warning.

What I propose is to keep what we have for now, i.e make the switch in arguments in 0.17.0, but keep the possibility of extending the warning by one release if we consider that it was too short.

Co-authored-by: Etienne Bacher <52219252+etiennebacher@users.noreply.github.com>
@eitsupi
Copy link
Collaborator Author

eitsupi commented Mar 26, 2024

What I propose is to keep what we have for now, i.e make the switch in arguments in 0.17.0, but keep the possibility of extending the warning by one release if we consider that it was too short.

I think it is worth doing so too.

@eitsupi eitsupi merged commit ddcf701 into main Mar 26, 2024
@eitsupi eitsupi deleted the fix-series branch March 26, 2024 10:06
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