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

feat(python): Rename Utf8 data type to String, keep Utf8 as alias #13257

Merged
merged 7 commits into from Dec 27, 2023

Conversation

stinodego
Copy link
Member

@stinodego stinodego commented Dec 27, 2023

I added the new String data type in three steps:

  1. Rename the data type, add the alias, and update internal references / PyO3 bindings.
  2. Run the test suite to make sure everything still works (with Utf8 used everywhere).
  3. Update Utf8 to String in test suite too.

@github-actions github-actions bot added enhancement New feature or an improvement of an existing feature python Related to Python Polars labels Dec 27, 2023
@stinodego stinodego marked this pull request as ready for review December 27, 2023 10:18
@ritchie46
Copy link
Member

Does pl.Utf8() return pl.String()? I think that it should.

@stinodego
Copy link
Member Author

stinodego commented Dec 27, 2023

Does pl.Utf8() return pl.String()? I think that it should.

No, it returns an object though that behaves exactly like String(). But I can make this change, there's something to be said for that. It's a very minor update - incoming :)

EDIT: Yeah if we accept that, things become much nicer 👍 we can just do Utf8 = String and export that. Just have to figure out how to nicely make Sphinx represent this.

@stinodego stinodego marked this pull request as draft December 27, 2023 10:23
@ritchie46
Copy link
Member

Great! That makes it behave more like an alias.

@stinodego
Copy link
Member Author

Yep, this is definitely better. Sphinx even picks this up automatically:

image

Should be good to go now!

@stinodego stinodego marked this pull request as ready for review December 27, 2023 10:54
Comment on lines -379 to +384
class Utf8(DataType):
class String(DataType):
"""UTF-8 encoded string type."""


# Allow Utf8 as an alias for String
Utf8 = String
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the core of this PR.

@ritchie46
Copy link
Member

Have you done the datatypes mentioned in the user-guide?

Feel free to merge this one. Especially now it is a proper alias it seems fine to me.

@stinodego
Copy link
Member Author

Have you done the datatypes mentioned in the user-guide?

Hadn't done that yet. Updated references in the user guide now. Will merge if CI is green!

@stinodego stinodego merged commit a75ad4c into main Dec 27, 2023
21 checks passed
@stinodego stinodego deleted the utf8-to-string-python branch December 27, 2023 14:51
@c-peters c-peters added the accepted Ready for implementation label Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Ready for implementation enhancement New feature or an improvement of an existing feature python Related to Python Polars
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants