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

gh-107801: Document SEEK_HOLE and SEEK_DATA #107936

Merged
merged 14 commits into from
Aug 17, 2023

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Aug 14, 2023

@erlend-aasland
Copy link
Contributor Author

I tried to keep this as simple as possible; since this is the reference, digressing deep into the explanation of the file hole/data concept and its use is out of place for a reference.

Copy link
Member

@AA-Turner AA-Turner left a comment

Choose a reason for hiding this comment

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

All good!

A

Doc/library/os.rst Outdated Show resolved Hide resolved
Co-authored-by: Adam Turner <[email protected]>
Doc/library/os.rst Outdated Show resolved Hide resolved
Doc/library/os.rst Outdated Show resolved Hide resolved
@erlend-aasland erlend-aasland requested a review from pitrou August 14, 2023 20:44
@erlend-aasland
Copy link
Contributor Author

Thanks for the reviews, Antoine and Adam; highly appreciated.

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

It is weird that the semantic of SEEK_HOLE and SEEK_DATA is described here, but the semantic of other three SEEK_* constants is described in other places. The latter in understandable, because it subtly differs for different types of streams. But I think that the documentation for all SEEK_* should be more unified.

  1. One way, do not document the semantic here, but only document it in lseek() description, in a single item list (or two similar item lists).

  2. Other way, add very general descriptions of traditional SEEK_* constants here (which works for all types of streams), more detailed description of new constants, and in the lseek() documentation and seek() methods documentation only document in details the behavior for traditional SEEK_* constants, and a reference for new SEEK_* constants.

In any case I think that changes in SEEK_* and lseek() documentations should be made in a single PR, because they are not independent.

@erlend-aasland
Copy link
Contributor Author

I think it is fine to document these in a separate PR. Small and incremental changes are aligns well with the recommendations of the Diátaxis, which we are using.

More important:
We could unify the SEEK_ constant docs, but I think they also make sense grouped like they are now. I'm not sure it matters much. The HOLE/DATA constants are pretty obscure, but then so is the whole tell/seek API.

Doc/library/os.rst Outdated Show resolved Hide resolved
@AA-Turner
Copy link
Member

I think it is fine to document these in a separate PR. Small and incremental changes ...

I agree with this, the previous PR got held up due to doing too much, I think these smaller PRs are easier -- noting that :const:`os.SEEK_HOLE` and :const:`os.SEEK_DATA` are currently referenced in the documentation.

A

@erlend-aasland erlend-aasland requested a review from pitrou August 15, 2023 20:40
@erlend-aasland
Copy link
Contributor Author

Thanks, @AA-Turner and @pitrou; PTAL.

@erlend-aasland
Copy link
Contributor Author

It looks to me all remarks have been addressed; I'm landing this later today, @pitrou. If further adjustments are needed, we can easily make a follow-up PR.

@erlend-aasland erlend-aasland merged commit 8a19f22 into python:main Aug 17, 2023
@miss-islington
Copy link
Contributor

Thanks @erlend-aasland for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

@erlend-aasland erlend-aasland deleted the docs/seek-hole-seek-data branch August 17, 2023 14:14
@bedevere-bot
Copy link

GH-108086 is a backport of this pull request to the 3.12 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.12 bug and security fixes label Aug 17, 2023
@bedevere-bot
Copy link

GH-108087 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Aug 17, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 17, 2023
(cherry picked from commit 8a19f22)

Co-authored-by: Erlend E. Aasland <[email protected]>
Co-authored-by: Adam Turner <[email protected]>
Co-authored-by: Antoine Pitrou <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 17, 2023
(cherry picked from commit 8a19f22)

Co-authored-by: Erlend E. Aasland <[email protected]>
Co-authored-by: Adam Turner <[email protected]>
Co-authored-by: Antoine Pitrou <[email protected]>
erlend-aasland added a commit that referenced this pull request Aug 17, 2023
(cherry picked from commit 8a19f22)

Co-authored-by: Erlend E. Aasland <[email protected]>
Co-authored-by: Adam Turner <[email protected]>
Co-authored-by: Antoine Pitrou <[email protected]>
Yhg1s pushed a commit that referenced this pull request Aug 17, 2023
gh-107801: Document SEEK_HOLE and SEEK_DATA (GH-107936)
(cherry picked from commit 8a19f22)

Co-authored-by: Erlend E. Aasland <[email protected]>
Co-authored-by: Adam Turner <[email protected]>
Co-authored-by: Antoine Pitrou <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants