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(python): Deprecate non-keyword args for some functions #6851

Merged
merged 12 commits into from
Feb 13, 2023

Conversation

stinodego
Copy link
Member

@stinodego stinodego commented Feb 13, 2023

Keyword-only strategy

My philosophy for marking arguments as keyword-only is as follows, roughly in order of priority:

1. If parameters clash, at least one of the parameters should be keyword-only.

  • Example: join(left_on, right_on, on) -> either on must be keyword-only, or left_on and right_on.
  • Reason: We want to avoid syntax like join(None, None, "a")

2. Boolean flags should always be keyword-only

  • Example: sort(reverse=False, nulls_last=False)
  • Reason 1: positional syntax is not very readable here, e.g. sort(True, False) -> which flags mean what?
  • Reason 2: flag parameters are relatively prone to re-ordering - this will not be breaking if the args are keyword-only

3. Keyword arguments that are rarely used or only used for very specific use cases should be keyword-only

  • Example: read_csv(sample_size=1024)
  • Reason: These arguments are more subject to reordering, renaming, or removal than other parameters. Having them keyword-only makes this easier to do.

4. Arguments after "single or sequence" arguments should be keyword-only

5. Newly added flags or experimental args should be keyword-only

  • Example: to_dummies(separator) (recent addition)
  • Reason: We don't always get it right straight away. It's easier to change or deprecate arguments when they are keyword-only.

Bonus. Arguments that do not match these criteria SHOULD NOT BE KEYWORD-ONLY.

  • Example: join(other, on, how, *, left_on, right_on)
  • Reason: There are valid use cases for positional arguments. -> For example, when arguments are passed as variables, the duplication can get ugly: join(other, on=on, how=how). Let users decide their preferred syntax in this case (keyword or positional).
  • If keyword-only arguments do not concretely help the user or the Polars developers, they should not be used.

Your input here is very welcome!

Changes

With this in mind, I made deprecation changes to the following functions/methods:

  • to_dummies
  • unique
  • concat_str
  • struct
  • join_asof
  • map
  • apply
  • cumfold
  • arg_sort_by
  • arg_sort
  • pivot

Next steps

There are definitely more to do. I figured I'd open this PR to run my 'philosphy' by you, and then do another pass with whatever strategy we come up with together.

I've also spotted quite a few parameter renaming opportunities, I'll do that next.

And then after that, I will do another PR where I remove the deprecations and apply the breaking changes. That can sit for a while until we go for 0.17.0.

@github-actions github-actions bot added python Related to Python Polars refactor Code improvements that do not change functionality labels Feb 13, 2023
@stinodego stinodego marked this pull request as ready for review February 13, 2023 14:24
@ritchie46
Copy link
Member

This seems a good improvement to me. 👍

@stinodego
Copy link
Member Author

All right, I'll go ahead and merge this and check the rest of the API for candidates according to the guidelines set out in this post.

If any issues pop up, the deprecations are easily reversed. Worst case scenario, some users will rewrite their stuff to use keyword arguments unnecessarily.

@stinodego stinodego merged commit e16c3eb into pola-rs:master Feb 13, 2023
@stinodego stinodego deleted the req-keywords-depr branch February 22, 2023 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python Related to Python Polars refactor Code improvements that do not change functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants