FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-83714: Use "stx_" prefix for all os.statx_result members by vstinner · Pull Request #140432 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

gh-83714: Use "stx_" prefix for all os.statx_result members - #140432

Merged
vstinner merged 5 commits into
python:mainfrom
vstinner:statx_stx
Oct 22, 2025
ConversationCommits (5)ChecksFiles changed
Merged

gh-83714: Use "stx_" prefix for all os.statx_result members#140432
vstinner merged 5 commits into
python:mainfrom
vstinner:statx_stx

Conversation

vstinner commented Oct 21, 2025 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Member

bedevere-app Bot mentioned this pull request Oct 21, 2025
Closed

vstinner commented Oct 21, 2025

Copy link
Copy Markdown
Member Author

I'm not convinced that it's a good idea to have os.statx_result which can be used instead of os.stat_result. It's not exactly the same, os.statx_result members can be None.

It's also strange that some statx members have a st_ prefix, whereas others have a stx_ prefix. I read the statx manual page and expect to have only stx_ prefix: same members names.

I don't think that stx_ prefix would prevent using statx() in pathlib in the future.

cc @jbosboom @cmaloney

cmaloney reviewed Oct 21, 2025
View reviewed changes

cmaloney left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'm 👍 on this as its evolved. There's too many weird edged cases and just separating entirely makes simpler to reason around.

I think if go forward with this all the stx_ attributes should be explicitly documented rather than refer to stat_result ones of different name. I'm often in a codebase wondering "what is this" and search in page to find; the text note "all the stat members but with a stx_ prefix" would break that

Comment thread Doc/library/os.rst Outdated
Comment thread Doc/library/os.rst Outdated

cmaloney Oct 21, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think this should list all the attributes now; doc having people translate in their head rather than see "here's the name I used" seems like it will break "search in page" finding the attributes.

vstinner Oct 21, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ok, I completed my PR to document all statx_members. It also clarifies which stat_result members are not part of statx_result:

  • st_creator
  • st_file_attributes
  • st_flags
  • st_fstype
  • st_gen
  • st_reparse_tag
  • st_rsize
  • st_type

Document all statx_result members
1f0a688 
Rename stx_birthtime to stx_btime, and rename stx_birthtime_ns to
stx_btime_ns.
Simplify posixmodule.c macros
12c49f7 
Remove now redundant MEMBER parameter.

vstinner commented Oct 21, 2025

Copy link
Copy Markdown
Member Author

I updated my PR to document all statx_result members, and I renamed stx_birthtime to stx_btime.

cmaloney reviewed Oct 21, 2025
View reviewed changes

cmaloney left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

👍 overall; curious @jbosboom thoughts. Appreciate the fixes/tweaks to Linux kernel version availability.

Comment thread Doc/library/os.rst

vstinner commented Oct 21, 2025

Copy link
Copy Markdown
Member Author

I changed my mind and just removed the comparison to stat_result:

statx_result has all the attributes that stat_result has on Linux, but using stx_ prefix instead of st_. statx_result is not a subclass of stat_result and cannot be used as a tuple.

I don't think that it's useful to compare these two structures, they are different and listing differences is not worth it.

jbosboom commented Oct 21, 2025

Copy link
Copy Markdown
Contributor

I think this needlessly complicates code that uses os.statx on Python >= 3.15 and falls back to os.stat on earlier versions, but I guess there won't be any user code doing that if we get the pathlib interface in 3.15 as well, so +0 from me.

@vstinner You've made substantial changes to both the interface and implementation of os.statx. I think you ought to add your name in the blurb for the issue.

vstinner requested a review from AA-Turner as a code owner October 22, 2025 08:33

vstinner commented Oct 22, 2025

Copy link
Copy Markdown
Member Author

@vstinner You've made substantial changes to both the interface and implementation of os.statx. I think you ought to add your name in the blurb for the issue.

Ok, I added myself to the credits.

vstinner merged commit 7339cf7 into python:main Oct 22, 2025
45 checks passed
vstinner deleted the statx_stx branch October 22, 2025 09:48
StanFromIreland pushed a commit to StanFromIreland/cpython that referenced this pull request Dec 6, 2025
pythongh-83714: Use "stx_" prefix for all os.statx_result members (py…
b4b780c 
…thon#140432)

Rename stx_birthtime to stx_btime, and rename stx_birthtime_ns to
stx_btime_ns.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

cmaloney cmaloney left review comments
AA-Turner Awaiting requested review from AA-Turner AA-Turner is a code owner

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL