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

RFC: rename PyString -> PyStr, and PyLong -> PyInt #4260

Closed
davidhewitt opened this issue Jun 18, 2024 · 12 comments · Fixed by #4370
Closed

RFC: rename PyString -> PyStr, and PyLong -> PyInt #4260

davidhewitt opened this issue Jun 18, 2024 · 12 comments · Fixed by #4370

Comments

@davidhewitt
Copy link
Member

We currently have PyString to represent Python str, and PyLong to represent Python int.

While PyString is reasonably clear, PyLong is primarily a historical accident based on the fact that Python 2's integers were split between int and long types, and so the Python C API is based on PyLongObject etc.

Interestingly, we do currently expose PyInt as pyo3::types::PyInt as an import alias.

Similar history applies to PyString; we currently have a PyUnicode type alias kicking around which is again a throwback to Python 2's str and unicode split (they became approximately bytes and str respectively in Python 3).

I would propose the following:

  • Rename PyString to PyStr, and leave PyString and PyUnicode as deprecated type aliases for a couple versions.
  • Similarly rename PyLong to PyInt, and leave PyLong as a deprecated type alias for a couple versions.

The only counter-arguments I can see:

  • PyString -> PyStr is not worth the churn. I could accept this if people hold this strongly, though I also think such a migration is a straightforward find-and-replace and improves alignment with the Python naming.
  • PyLong -> PyInt is unappealing because we build upon PyLong_ C APIs. After some consideration I find this a non-argument; PyInt is a much clearer name in my opinion, and I don't think we would ever consider PyUnicode as better than PyStr or PyString. So by extension I see no reason to prefer C-API consistency of PyLong over PyInt.

Subject to agreement from maintainers that we want to make this change, I think this is a relatively easy change to implement, so tentatively marking as "Good First Issue".

@LilyFoote
Copy link
Contributor

Makes sense to me!

@Stargateur
Copy link

for me PyString make sense cause it's allocate.

@davidhewitt
Copy link
Member Author

for me PyString make sense cause it's allocate.

I do like this argument tbh; and if we chose to leave PyString as-is and just update PyLong -> PyInt, this argument would be a somewhat reasonable justification (alongside the benefit of avoiding churn).

@Icxolu
Copy link
Contributor

Icxolu commented Jun 18, 2024

I also think PyString is fine as is. PyStr can also make the impression that is would somehow be coupled to Rust's &str, which it is not really. Renaming PyLong to PyInt sounds reasonable to me, given that long is neither a thing in Rust or Python, and PyInt makes is clearer what it refers to.

@davidhewitt
Copy link
Member Author

It sounds like the conclusion here is PyString will stay as-is (we can deprecate the PyUnicode alias, I think, though).

On the other hand, we have broad agreement that PyLong -> PyInt is desirable.

Just need someone to find a moment implement, in that case 👍

@shreyash2002
Copy link

Hi @davidhewitt
I would love to do changes for PyLong -> PyInt.

I'm still a beginner in rust & open-source. So, this issue would be a good starting point for me

@davidhewitt
Copy link
Member Author

Please feel free to go for it! I suggest starting with a PR and we can review and help iterate that way.

shreyash2002 added a commit to shreyash2002/pyo3 that referenced this issue Jun 25, 2024
-  PyLong is primarily a historical accident based on the fact that Python 2's integers were split between int and long types, and so the Python C API is based on PyLongObject etc. 

- So, renaming it to PyInt
shreyash2002 added a commit to shreyash2002/pyo3 that referenced this issue Jun 25, 2024
-PyLong is primarily a historical accident based on the fact that Python 2's integers were split between int and long types, and so the Python C API is based on PyLongObject etc. 

- So, renaming it to PyInt
shreyash2002 added a commit to shreyash2002/pyo3 that referenced this issue Jun 25, 2024
shreyash2002 added a commit to shreyash2002/pyo3 that referenced this issue Jun 25, 2024
@Cheukting
Copy link
Contributor

@davidhewitt is this issue ready to be closed? Seems the PR has been merged

@mejrs
Copy link
Member

mejrs commented Jul 20, 2024

@davidhewitt is this issue ready to be closed? Seems the PR has been merged

David mentioned deprecating the PyUnicode alias, that sounds like good idea to me :)

@davidhewitt
Copy link
Member Author

Yes I'd still like to see that deprecated. Should be a fairly straightforward PR for someone able to find a moment 😁

@Cheukting
Copy link
Contributor

I see, I am happy to work on deprecating the PyUnicode alias, do we need to produce a warning or what's the plan for deprecating it other than removing it from the code?

@davidhewitt
Copy link
Member Author

We can use the #[deprecated] attribute on a type alias like we did in #4347

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants