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): Rename utils module to _utils to explicitly mark it as private #14772

Merged
merged 5 commits into from
Mar 1, 2024

Conversation

stinodego
Copy link
Member

@stinodego stinodego commented Feb 29, 2024

As discussed on Discord.

In our deprecation policy, we explicitly state that changing internal module paths is not considered a breaking change. To avoid surprises for users that do not read the policy, I am explicitly marking the utils module as private.

I did notice that the _get_shared_lib_location util is used in the plugins docs. It is not used internally. What is the deal with this (private) function and should we make it public? @MarcoGorelli would appreciate your input!

@github-actions github-actions bot added internal An internal refactor or improvement python Related to Python Polars labels Feb 29, 2024
@stinodego stinodego marked this pull request as ready for review February 29, 2024 13:40
@MarcoGorelli
Copy link
Collaborator

MarcoGorelli commented Feb 29, 2024

I had it in my backlog to ask about making _get_shared_lib_location public, so thanks for bringing this up 😄

For plugins, the other thing I'd love would be a public parse_into_expr function - currently I've put this one in the cookiecutter, which only uses public methods, but it would be nice to just do

from polars.plugin_utils import (
    get_shared_lib_location,
    parse_into_expr,
)

https://github.com/MarcoGorelli/cookiecutter-polars-plugins/blob/6071d14e88deebea4c73cbbb8d63b7d09d9f1da1/%7B%7B%20cookiecutter.project_slug%20%7D%7D/%7B%7B%20cookiecutter.project_slug%20%7D%7D/utils.py#L11-L48


Having said that, with this change, all plugins will break, even though authors just followed official guidance. I'd really appreciate it if

from polars.utils.udfs import _get_shared_lib_location

could keep working for now, but just emit a deprecation warning, advising to use whatever the replacement path is. Given that polars.api already has register_expr_namespace, maybe that would be a good place for this to live as well?

Copy link

codecov bot commented Feb 29, 2024

Codecov Report

Attention: Patch coverage is 93.44262% with 36 lines in your changes are missing coverage. Please review.

Project coverage is 80.93%. Comparing base (d482bce) to head (e2944ab).
Report is 2 commits behind head on main.

❗ Current head e2944ab differs from pull request most recent head dccec10. Consider uploading reports for the commit dccec10 to get more accurate results

Files Patch % Lines
py-polars/polars/_utils/udfs.py 91.62% 19 Missing and 12 partials ⚠️
py-polars/polars/utils/udfs.py 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14772      +/-   ##
==========================================
- Coverage   80.94%   80.93%   -0.01%     
==========================================
  Files        1324     1325       +1     
  Lines      171941   171904      -37     
  Branches     2450     2452       +2     
==========================================
- Hits       139176   139129      -47     
- Misses      32296    32305       +9     
- Partials      469      470       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stinodego
Copy link
Member Author

Having said that, with this change, all plugins will break, even though authors just followed official guidance.

All right, will keep that unchanged in this PR. Can follow up with a PR to add a polars.plugins module with some of the relevant utils.

@MarcoGorelli
Copy link
Collaborator

awesome, thanks! I'll repurpose #14779 as a follow-up then once this is in

@stinodego stinodego merged commit b8d7a0f into main Mar 1, 2024
24 checks passed
@stinodego stinodego deleted the utils-privacy branch March 1, 2024 08:02
@c-peters c-peters added the accepted Ready for implementation label Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Ready for implementation internal An internal refactor or improvement python Related to Python Polars
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants