bpo-31368: Enhance os.preadv() documentation #5415

vstinner · 2018-01-29T08:19:46Z

https://bugs.python.org/issue31368

vstinner · 2018-01-29T08:21:27Z

@pablogsal: would you mind to review my PR?

csabella · 2018-01-29T15:30:11Z

Doc/library/os.rst

+   Write the *buffers* contents to file descriptor *fd* at offset *offset*.
+   *buffers* must be a sequence of :term:`bytes-like objects <bytes-like
+   object>`. Buffers are processed in array order. Entire contents of first
+   buffer is written before proceeding to second, and so on.


'Entire contents of the first buffer'
'before proceeding to the second'

pablogsal · 2018-01-29T16:39:25Z

Doc/library/os.rst

-   (sysconf() value SC_IOV_MAX) on the number of buffers that can be used.
+   processed in array order. Entire contents of the first buffer is written
+   before proceeding to the second, and so on. The operating system may set a
+   limit (sysconf() value SC_IOV_MAX) on the number of buffers that can be


It will make sense to change

sysconf() -> :func:`sysconf`

and

SC_IOV_MAX -> ``'SC_IOV_MAX'``

?

Oh wait, I didn't want to modify this documentation... But then I read the doc, and oh, all these documentations can be enhanced...

pread(): rename second parameter from 'buffersize' to 'n', reuse read() parameter name. The parameter is positional-only and so this change is backward compatible.

vstinner · 2018-01-29T17:15:03Z

Sorry for reviewers, but I basically rewrote my whole PR to cleanup the documentation of all read and write functions:

read, pread, readv, preadv
write, pwrite, writev, pwritev

The documentation should now be more consistent.

YoSTEALTH · 2018-01-29T16:40:56Z

Doc/library/os.rst

+   - :data:`RWF_HIPRI`
+   - :data:`RWF_NOWAIT`
+
+   Using non-zero flags requires Linux 4.6 or newer.


could be change this to: "Using non-zero flags requires preadv2 support on Linux 4.7 or newer."

I'm trying to avoid mentioning preadv2(). Do you think that you must mention it?

All the flags actually belong to preadv2() function though, maybe a tiny mention wont hurt?
Its your call.

You should absolutely mention preadv2 and pwritev2. At the point someone is using these features in the os module they're also reading man pages and lwn articles, and you want to help them make the connection. If someone wonders whether python has a wrapper for preadv2 then searching for that in the docs should find os.preadv.

YoSTEALTH · 2018-01-29T16:43:26Z

Doc/library/os.rst

+   - :data:`RWF_DSYNC`
+   - :data:`RWF_SYNC`

   Using non-zero flags requires Linux 4.7 or newer.


could be also change this to: "Using non-zero flags requires pwritev2 support on Linux 4.7 or newer."

YoSTEALTH · 2018-01-29T17:12:57Z

Doc/library/os.rst

+   The operating system may set a limit (:func:`sysconf` value
+   ``'SC_IOV_MAX'``) on the number of buffers that can be used.
+
+   Availability: Linux 2.6.30 abd newer, FreeBSD 6.0 and newer,


should be "Availability: Linux 2.6.30, FreeBSD 6.0 and newer."

"and newer" is commonly used in os.rst documentation (i fixed the typo)

vadmium · 2018-01-30T07:16:45Z

Doc/library/os.rst

+   The operating system may set a limit (:func:`sysconf` value
+   ``'SC_IOV_MAX'``) on the number of buffers that can be used.
+
+   Combine the functionality of :func:`os.readv` and :func:`os.pread`.


This reads as an instruction, but it is no longer clear it is for the implementation. Maybe drop the imperative and write “This function combines the functionality . . .”. Also applies to pwritev below.

vadmium · 2018-01-30T07:16:46Z

Doc/library/os.rst

+   - :data:`RWF_NOWAIT`
+
+   Return the total number of bytes actually read which can be less than the
+   total capacity of all the objects.


Needs a comma: “. . . number of bytes read, which can be less . . .”. Applies to readv below too.

vadmium · 2018-01-30T07:23:15Z

Doc/library/os.rst

+
+   If some data was successfully read, it will return the number of bytes read.
+   If no bytes were read, it will return ``-1`` and set errno to
+   :data:`errno.EAGAIN`.


I’m confused. Normally you access errno as an attribute of an exception. Do the Python wrapper(s) return −1 or raise OSError/BlockingIOError?

vadmium · 2018-01-30T07:36:30Z

Doc/library/os.rst

-   (sysconf() value SC_IOV_MAX) on the number of buffers that can be used.
-   :func:`~os.pwritev` writes the contents of each object to the file descriptor
-   and returns the total number of bytes written.
+   Write the *buffers* contents to file descriptor *fd* at a offset *offset*,


a → an or the, or even better “at offset offset” would work I think.

vstinner · 2018-02-01T02:37:05Z

I abandon my change. Feel free to reuse my change to create a new PR if you want to enhance the documentation.

bpo-31368: Enhance os.preadv() documentation

52b4da1

the-knights-who-say-ni added the CLA signed label Jan 29, 2018

bedevere-bot added the awaiting merge label Jan 29, 2018

vstinner added the skip news label Jan 29, 2018

What's New: describe added functions

5809a57

csabella reviewed Jan 29, 2018

View reviewed changes

vstinner added 2 commits January 29, 2018 16:37

Adding missing "the" in the doc

14eafd2

Move preadv to fix ordering

622a8ec

pablogsal requested changes Jan 29, 2018

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting merge labels Jan 29, 2018

Cleanup other read/write docs

40457cb

pread(): rename second parameter from 'buffersize' to 'n', reuse read() parameter name. The parameter is positional-only and so this change is backward compatible.

YoSTEALTH suggested changes Jan 29, 2018

View reviewed changes

vadmium reviewed Jan 30, 2018

View reviewed changes

vstinner closed this Feb 1, 2018

vstinner deleted the preadv_doc branch May 29, 2018 22:31

Uh oh!

bpo-31368: Enhance os.preadv() documentation #5415

bpo-31368: Enhance os.preadv() documentation #5415

Uh oh!

Conversation

vstinner commented Jan 29, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Jan 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablogsal Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Jan 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Feb 1, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

vstinner commented Jan 29, 2018 •

edited by bedevere-bot

Loading

pablogsal Jan 29, 2018 •

edited

Loading