gh-107801: Improve io.*.seek docs and docstrings #107899

erlend-aasland · 2023-08-12T22:08:12Z

Name positional parameters consistently, where possible: seek(offset, whence, /)
Add param docstrings to _io._IOBase.seek, so the various
implementations interit them.
~~Use io.SEEK_*, not os.SEEK_* or magical numbers.~~
Document io.SEEK_*
Override docstrings for subclasses where offset must be zero for
SEEK_CUR and SEEK_END.

Issue: Docs and docstrings for io.*.seek are inconsistent #107801

📚 Documentation preview 📚: https://cpython-previews--107899.org.readthedocs.build/

- Name positional parameters consistently: seek(offset, whence, /) - Add param docstrings to _io._IOBase.seek, so the various implementations interit them. - Use io.SEEK_*, not os.SEEK_* - Override docstrings for subclasses where 'offset' must be zero for SEEK_CUR and SEEK_END.

erlend-aasland · 2023-08-12T22:34:22Z

FTR, this won't backport cleanly.

erlend-aasland · 2023-08-12T22:39:53Z

~~We should land #107903 first.~~ OTOH, the diff for #107903 will be smaller if we land this first.

erlend-aasland · 2023-08-13T07:15:12Z

Doc/tutorial/inputoutput.rst

+the *whence* argument.
+A *whence* value of :data:`io.SEEK_SET` measures from the beginning of the file,
+:data:`io.SEEK_CUR` uses the current file position,
+and :data:`io.SEEK_END` uses the end of the file as the reference point.
+*whence* can be omitted and defaults to :data:`!io.SEEK_SET`,
+using the beginning of the file as the reference point. ::


I do find it strange that the tutorial delves into weird low-level APIs like seek. I can't even remember the last time I needed seek() for a real use case.

I think it's reasonable to show that it's possible, especially after covering tell.

Yes, if we introduce tell, surely we must also talk about seek. But I think an "how to do advanced I/O" is more fitting for these APIs.

Well, conceptually it's not really advanced nor is it difficult to understand.

The concept is easy enough to grasp. The APIs are a pain, though.

erlend-aasland · 2023-08-13T07:56:23Z

FTR, this won't backport cleanly.

I'm not sure we should backport the clinic changes; perhaps we should only backport the Doc/ changes.

AA-Turner

For an innocuous method, seek really is a can of worms... I've made some suggestions on wording, and potentially lifting a few items out of docstrings into the documentation. Thanks for doing the hard work here!

A

AA-Turner · 2023-08-13T10:06:28Z

Doc/library/io.rst

+
+   .. seealso::
+
+      :data:`os.SEEK_SET`, :data:`os.SEEK_CUR`, and :data:`os.SEEK_END`.


It seems the os constants were added in 2.5 for the os.lseek function (and io's in 2.7 & 3.1 (#48822)). Given the values are the same there's little difference, but should we provide which module's constants you're 'meant' to use in which cases?

Edit: Note that fnctl.lockf, mmap.seek and sqlite3.Blob.seek use the os constants.

As os constants were added earlier, I think we can just refer to them from here.

I have no opinion on which constants to use, but my gut reaction would be to use the constants from io for io stuff.

Doc/library/io.rst

AA-Turner · 2023-08-13T10:23:13Z

Doc/library/io.rst

+      * :data:`SEEK_CUR` -- seek from current stream position;
+        *offset* may be negative
+      * :data:`SEEK_END` -- seek from the end of the stream;
+        *offset* is usually negative



Given the pitfalls of seeking with files in text-mode, perhaps IOBase.seek should link to TextIOBase.seek, which has more specific information on the limitations here:

Suggested change

.. note::

When working with a text stream :py:meth:`!seek` has limitations

beyond the general interface described here,

which are described in more detail in :py:meth:`TextIOBase.seek`.

Doc/library/io.rst

AA-Turner · 2023-08-13T10:30:18Z

Doc/library/io.rst

+      * :data:`SEEK_SET` -- seek from the start of the stream (the default);
+        *offset* must either be a number returned by
+        :meth:`TextIOBase.tell`, or zero.
+        Any other *offset* value produces undefined behaviour.
+      * :data:`SEEK_CUR` -- seek from current stream position;
+        *offset* must be zero, which is a no-operation.
+        All other values are unsupported.
+      * :data:`SEEK_END` -- seek from the end of the stream;
+        *offset* must be zero.
+        All other values are unsupported.


Suggested change

* :data:`SEEK_SET` -- seek from the start of the stream (the default);

*offset* must either be a number returned by

:meth:`TextIOBase.tell`, or zero.

Any other *offset* value produces undefined behaviour.

* :data:`SEEK_CUR` -- seek from current stream position;

*offset* must be zero, which is a no-operation.

All other values are unsupported.

* :data:`SEEK_END` -- seek from the end of the stream;

*offset* must be zero.

All other values are unsupported.

* :data:`SEEK_SET` -- seek from the start of the stream (the default);

*offset* must either be a number returned by

:meth:`TextIOBase.tell`, or zero.

Any other *offset* value produces undefined behaviour.

* :data:`SEEK_CUR` -- seek from the current stream position;

*offset* must be zero, which does not change the stream position.

All other values are unsupported, and will raise an exception.

* :data:`SEEK_END` -- seek from the end of the stream;

*offset* must be zero, which does not change the stream position.

All other values are unsupported, and will raise an exception.

I think this is better:

Suggested change

* :data:`SEEK_SET` -- seek from the start of the stream (the default);

*offset* must either be a number returned by

:meth:`TextIOBase.tell`, or zero.

Any other *offset* value produces undefined behaviour.

* :data:`SEEK_CUR` -- seek from current stream position;

*offset* must be zero, which is a no-operation.

All other values are unsupported.

* :data:`SEEK_END` -- seek from the end of the stream;

*offset* must be zero.

All other values are unsupported.

* :data:`SEEK_SET` -- seek from the start of the stream (the default);

*offset* must either be a number returned by

:meth:`TextIOBase.tell`, or zero.

Any other *offset* value produces undefined behaviour.

* :data:`SEEK_CUR` -- seek from the current stream position;

*offset* must be zero, leaving the stream position unchanged.

All other values are unsupported, and will raise an exception.

* :data:`SEEK_END` -- seek from the end of the stream;

*offset* must be zero, leaving the stream position at the end of the stream.

All other values are unsupported, and will raise an exception.

AA-Turner · 2023-08-13T10:33:50Z

Modules/_io/iobase.c


-Return the new absolute position.
+Note that not all file objects are seekable.


We should include this admonition in the documentation for IOBase.seek (with reference to seekable()?)

Sounds good to me.

Modules/_io/bytesio.c

AA-Turner · 2023-08-13T10:36:37Z

Modules/_io/textio.c

+
+Change the file position and return the new absolute position.
+
+Offsets relative to the start of the file are usually zero or positive.


For TextIOWrapper only the opaque int returned by tell() is valid -- would that ever be negative?

AA-Turner · 2023-08-13T10:38:27Z

Modules/_io/stringio.c

-Returns the new absolute position.
+Offsets relative to the start of the stream are usually zero or positive.
+Offsets relative to the current and end of file positions must zero;
+any other values are unsupported, and will raise OSError.


Should we add this to the documentation for StringIO, as it is different from the TextIOBase behaviour (raising OSError in the stead of UnsupportedOperation)?

I think that that it may be easier to remove the mention of exception type. Not all should be included in docstrings. For more details users can look in the documentation.

serhiy-storchaka

I just started working on it. Good thing I didn't advance far before your request for review.

But I noticed few things which you did not change in this PR, so I included them in comments.

serhiy-storchaka · 2023-08-13T14:28:18Z

Doc/library/io.rst

@@ -168,6 +168,19 @@ High-level Module Interface
   :func:`os.stat`) if possible.


+.. data:: SEEK_SET


These constants are defined in os and io modules. Instead of duplicating the description, I think that it is better to document them in one place and refer to them from other module.

They are exposed API in both modules, so in my opinion they should be documented in both io and os docs. I think .. seealso:: for cross linking is fine.

serhiy-storchaka · 2023-08-13T14:30:18Z

Doc/library/io.rst

+
+   .. seealso::
+
+      :data:`os.SEEK_SET`, :data:`os.SEEK_CUR`, and :data:`os.SEEK_END`.


As os constants were added earlier, I think we can just refer to them from here.

serhiy-storchaka · 2023-08-13T14:37:59Z

Doc/library/io.rst


-      * :data:`SEEK_SET` or ``0`` -- start of the stream (the default);
+      * :data:`SEEK_SET` -- seek from the start of the stream (the default);


Please keep the literal values. They are standard and widely used. Removing the literal values will make the reader confused when they see file.seek(0, 2) in someone's other code.

The text above contains: "offset is interpreted relative to the position indicated by whence." The text in this paragraph describes that position. So if you consider it as a continuation of the former phrase, "seek from" may be not needed.

Please keep the literal values. They are standard and widely used. Removing the literal values will make the reader confused when they see file.seek(0, 2) in someone's other code.

Good call. I agree.

I would also keep the literal values.

serhiy-storchaka · 2023-08-13T14:49:36Z

Doc/library/io.rst

+      * :data:`SEEK_CUR` -- seek from current stream position;
+        *offset* may be negative
+      * :data:`SEEK_END` -- seek from the end of the stream;
+        *offset* is usually negative


Suggested change

*offset* is usually negative

*offset* is usually zero or negative

serhiy-storchaka · 2023-08-13T14:56:00Z

Doc/library/io.rst

-        *offset* must be zero (all other values are unsupported).
-
-      Return the new absolute position as an opaque number.
+      Change the stream position to the given byte *offset*,


It is not a byte offset.

I suggest also to change the name of the first argument to something other. For example to "cookie" as it is use in the code. Or to "pos" as it was in older documentation. But "offset" is misleading.

Removing references to the name further in the documentation and using "the first argument" instead of the name if it is impossible can also help, because it is hard to came with good name.

The existing docs/docstrings use offset, cookie, and pos. I don't understand what's meant by cookie. I understand what's meant by offset or position. If there is no need to use the same name, there is no need for this PR.

I think offset is fine, as it is still an offset into the file even if not an integer byte offset. I agree we should stay away from cookie et al.

"Cookie" means an opaque value. No, it is not an offset. It is several values (position of the buffer in the file, position in the buffer, state of the encoder and decoder, packed in a 257-bit integer. See code of _pack_cookie() and _unpack_cookie() and very complex code of tell() and seek() in _pyio.py.

This PR is needed because semantic of tell() and seek() in binary and text files is so different.

Ouch, yes TextIOWrapper is a different thing. Very unfortunate.

serhiy-storchaka · 2023-08-13T15:22:27Z

Modules/_io/stringio.c

-Returns the new absolute position.
+Offsets relative to the start of the stream are usually zero or positive.
+Offsets relative to the current and end of file positions must zero;
+any other values are unsupported, and will raise OSError.


I think that that it may be easier to remove the mention of exception type. Not all should be included in docstrings. For more details users can look in the documentation.

serhiy-storchaka · 2023-08-13T15:23:06Z

Modules/_io/stringio.c

+    offset as pos: Py_ssize_t
+    whence: int(c_default='0') = io.SEEK_SET


You should also copy parameter docstrings. Otherwise the function docstring will look incomplete.

The param docstrings are rendered to be a part of the docstring during clinic input parsing. It will be part of the whole docstring after make clinic. The subclasses will inherit this, unless they override the docstring explicitly.

No. Subclasses "inherit" docstrings only if they do not have their own docstrings.

I put "inherit" in quotes because actually they do not do this. It is pydoc who falls back to looking up docstrings in the parent classes.

Look at the Argument Clinic generated docstrings and you will see that they are incomplete.

docstrings as of this PR:

Seek docs for <class '_io.BufferedRWPair'>: BufferedRWPair.__text_signature__: ($self, offset, whence=io.SEEK_SET, /) BufferedRWPair.__doc__: Change the stream position and return the new absolute position. offset Offset as byte count. Relative to the position given by 'whence'. whence Relative position, used by 'offset'. Valid values are: * io.SEEK_SET -- Start of stream (the default) * io.SEEK_CUR -- Current position * io.SEEK_END -- End of stream Offsets relative to the start of the stream are usually zero or positive, offsets relative to the current stream position may be zero, positive or negative, and offsets relative to the end of the stream are usually zero or negative. Some platforms allow seeking beyond the end of a file. Note that not all file objects are seekable. Seek docs for <class '_io.BufferedRandom'>: BufferedRandom.__text_signature__: ($self, offset, whence=io.SEEK_SET, /) BufferedRandom.__doc__: None Seek docs for <class '_io.BufferedReader'>: BufferedReader.__text_signature__: ($self, offset, whence=io.SEEK_SET, /) BufferedReader.__doc__: None Seek docs for <class '_io.BufferedWriter'>: BufferedWriter.__text_signature__: ($self, offset, whence=io.SEEK_SET, /) BufferedWriter.__doc__: None Seek docs for <class '_io.BytesIO'>: BytesIO.__text_signature__: ($self, offset, whence=io.SEEK_SET, /) BytesIO.__doc__: Change stream position and return the new absolute position. Set the byte offset 'offset', relative to position indicated by 'whence': Seek docs for <class '_io.FileIO'>: FileIO.__text_signature__: ($self, offset, whence=io.SEEK_SET, /) FileIO.__doc__: None Seek docs for <class '_io.StringIO'>: StringIO.__text_signature__: ($self, offset, whence=io.SEEK_SET, /) StringIO.__doc__: Change the stream position and return the new absolute position. Offsets relative to the start of the stream are usually zero or positive. Offsets relative to the current and end of file positions must zero; any other values are unsupported, and will raise OSError. Some platforms allow seeking beyond the end of a file. Note that not all file objects are seekable. Seek docs for <class '_io.TextIOWrapper'>: TextIOWrapper.__text_signature__: ($self, offset, whence=io.SEEK_SET, /) TextIOWrapper.__doc__: Change the file position and return the new absolute position. Offsets relative to the start of the file are usually zero or positive. Offsets relative to the current and end of file positions must zero; any other values are unsupported, and will raise the io.UnsupportedOperation exception. Some platforms allow seeking beyond the end of a file. Note that not all file objects are seekable. Seek docs for <class '_io._BufferedIOBase'>: _BufferedIOBase.__text_signature__: ($self, offset, whence=io.SEEK_SET, /) _BufferedIOBase.__doc__: Change the stream position and return the new absolute position. offset Offset as byte count. Relative to the position given by 'whence'. whence Relative position, used by 'offset'. Valid values are: * io.SEEK_SET -- Start of stream (the default) * io.SEEK_CUR -- Current position * io.SEEK_END -- End of stream Offsets relative to the start of the stream are usually zero or positive, offsets relative to the current stream position may be zero, positive or negative, and offsets relative to the end of the stream are usually zero or negative. Some platforms allow seeking beyond the end of a file. Note that not all file objects are seekable. Seek docs for <class '_io._IOBase'>: _IOBase.__text_signature__: ($self, offset, whence=io.SEEK_SET, /) _IOBase.__doc__: Change the stream position and return the new absolute position. offset Offset as byte count. Relative to the position given by 'whence'. whence Relative position, used by 'offset'. Valid values are: * io.SEEK_SET -- Start of stream (the default) * io.SEEK_CUR -- Current position * io.SEEK_END -- End of stream Offsets relative to the start of the stream are usually zero or positive, offsets relative to the current stream position may be zero, positive or negative, and offsets relative to the end of the stream are usually zero or negative. Some platforms allow seeking beyond the end of a file. Note that not all file objects are seekable. Seek docs for <class '_io._RawIOBase'>: _RawIOBase.__text_signature__: ($self, offset, whence=io.SEEK_SET, /) _RawIOBase.__doc__: Change the stream position and return the new absolute position. offset Offset as byte count. Relative to the position given by 'whence'. whence Relative position, used by 'offset'. Valid values are: * io.SEEK_SET -- Start of stream (the default) * io.SEEK_CUR -- Current position * io.SEEK_END -- End of stream Offsets relative to the start of the stream are usually zero or positive, offsets relative to the current stream position may be zero, positive or negative, and offsets relative to the end of the stream are usually zero or negative. Some platforms allow seeking beyond the end of a file. Note that not all file objects are seekable. Seek docs for <class '_io._TextIOBase'>: _TextIOBase.__text_signature__: ($self, offset, whence=io.SEEK_SET, /) _TextIOBase.__doc__: Change the stream position and return the new absolute position. offset Offset as byte count. Relative to the position given by 'whence'. whence Relative position, used by 'offset'. Valid values are: * io.SEEK_SET -- Start of stream (the default) * io.SEEK_CUR -- Current position * io.SEEK_END -- End of stream Offsets relative to the start of the stream are usually zero or positive, offsets relative to the current stream position may be zero, positive or negative, and offsets relative to the end of the stream are usually zero or negative. Some platforms allow seeking beyond the end of a file. Note that not all file objects are seekable.

Look at the Argument Clinic generated docstrings and you will see that they are incomplete.

Of course. Only the _IOBase docstring will be generated completely with param docstrings inlined, as the PR stands now.

Perhaps param docstrings are not a good fit for these functions, though.

All docstrings should be complete. If possible, make them empty, then pydoc will show the docstring from the parent class. Perhaps you only need two docstrings: for binary and text streams.

serhiy-storchaka · 2023-08-13T15:24:44Z

Modules/_io/stringio.c

+Offsets relative to the current and end of file positions must zero;
+any other values are unsupported, and will raise OSError.
+
+Some platforms allow seeking beyond the end of a file.


Is the behavior of StringIO platform-depending?

No, this is a copy and paste error. Thanks for noticing.

serhiy-storchaka · 2023-08-13T15:25:09Z

Modules/_io/stringio.c

+
+Some platforms allow seeking beyond the end of a file.
+
+Note that not all file objects are seekable.


This is StringIO.

serhiy-storchaka · 2023-08-13T15:26:58Z

Modules/_io/textio.c

+any other values are unsupported, and will raise the
+io.UnsupportedOperation exception.
+
+Some platforms allow seeking beyond the end of a file.


It is not related to TextIOWrapper.

serhiy-storchaka · 2023-08-13T15:33:07Z

There may be an open issue about unification of parameter names of seek(). Try to find it and look if there is some discussion.

erlend-aasland · 2023-08-14T07:02:07Z

There may be an open issue about unification of parameter names of seek(). Try to find it and look if there is some discussion.

I'll try to find it. I'll also mark this as a draft PR; there is a lot of discussions to land before proceeding with this work.

pitrou · 2023-08-14T07:10:18Z

Modules/_io/bytesio.c

-     1  Current position - pos may be negative;
-     2  End of stream - pos usually negative.
-Returns the new absolute position.
+Set the byte offset 'offset', relative to position indicated by 'whence':


I think it would be better to keep a consistent terminology instead of alternating between "offset" and "position". "position" is probably more easily understood IMHO.

Position is used to describe the whence param. Would it not be confusing if 'position' is used to describe both parameters?

Well the summary string does say "Change stream position and return the new absolute position". In the next line "stream position" suddenly becomes "byte offset".

But this could also be "Set the stream position to the byte 'offset', relative to ...".

Suggested change

Set the byte offset 'offset', relative to position indicated by 'whence':

Set the stream position to the byte offset 'offset', relative to the position indicated by 'whence':

erlend-aasland · 2023-08-14T07:43:24Z

I think this PR tries to do too much. There will be too many scattered discussions. I'll open smaller and more focused PRs; I'll tag you for review. I'll start with the low-hanging fruit.

erlend-aasland added the skip news label Aug 12, 2023

bedevere-bot mentioned this pull request Aug 12, 2023

Docs and docstrings for io.*.seek are inconsistent #107801

Closed

offset is int

8bd646f

erlend-aasland requested a review from AA-Turner August 12, 2023 22:34

erlend-aasland marked this pull request as ready for review August 13, 2023 06:59

bedevere-bot added the awaiting core review label Aug 13, 2023

Use C default 0; we cannot guarantee what SEEK_SET is on C level

4fc2095

erlend-aasland commented Aug 13, 2023

View reviewed changes

erlend-aasland requested a review from vstinner August 13, 2023 07:41

erlend-aasland added needs backport to 3.11 only security fixes needs backport to 3.12 bug and security fixes labels Aug 13, 2023

AA-Turner reviewed Aug 13, 2023

View reviewed changes

AA-Turner added topic-IO docs Documentation in the Doc dir labels Aug 13, 2023

erlend-aasland requested a review from serhiy-storchaka August 13, 2023 13:30

serhiy-storchaka reviewed Aug 13, 2023

View reviewed changes

rhettinger requested a review from pitrou August 13, 2023 19:49

erlend-aasland marked this pull request as draft August 14, 2023 07:02

bedevere-bot removed the awaiting core review label Aug 14, 2023

pitrou reviewed Aug 14, 2023

View reviewed changes

erlend-aasland closed this Aug 14, 2023

erlend-aasland deleted the docs/seek branch August 14, 2023 07:43


		.. seealso::

		:data:`os.SEEK_SET`, :data:`os.SEEK_CUR`, and :data:`os.SEEK_END`.

+      .. note::
+         When working with a text stream :py:meth:`!seek` has limitations
+         beyond the general interface described here,
+         which are described in more detail in :py:meth:`TextIOBase.seek`.


		Return the new absolute position.
		Note that not all file objects are seekable.


		Change the file position and return the new absolute position.

		Offsets relative to the start of the file are usually zero or positive.

		@@ -168,6 +168,19 @@ High-level Module Interface
		:func:`os.stat`) if possible.


		.. data:: SEEK_SET


		* :data:`SEEK_SET` or ``0`` -- start of the stream (the default);
		* :data:`SEEK_SET` -- seek from the start of the stream (the default);

	offset is usually negative
	offset is usually zero or negative

		offset as pos: Py_ssize_t
		whence: int(c_default='0') = io.SEEK_SET


		Some platforms allow seeking beyond the end of a file.

		Note that not all file objects are seekable.

	Set the byte offset 'offset', relative to position indicated by 'whence':
	Set the stream position to the byte offset 'offset', relative to the position indicated by 'whence':

gh-107801: Improve io.*.seek docs and docstrings #107899

gh-107801: Improve io.*.seek docs and docstrings #107899

Conversation

erlend-aasland commented Aug 12, 2023 • edited Loading

erlend-aasland commented Aug 12, 2023

erlend-aasland commented Aug 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erlend-aasland commented Aug 13, 2023

AA-Turner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erlend-aasland Aug 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erlend-aasland Aug 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serhiy-storchaka commented Aug 13, 2023

erlend-aasland commented Aug 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erlend-aasland commented Aug 14, 2023

erlend-aasland commented Aug 12, 2023 •

edited

Loading

erlend-aasland commented Aug 12, 2023 •

edited

Loading

erlend-aasland Aug 14, 2023 •

edited

Loading

erlend-aasland Aug 14, 2023 •

edited

Loading