Skip to content

Conversation

@1st1
Copy link
Member

@1st1 1st1 commented Sep 14, 2018

@bedevere-bot bedevere-bot added docs Documentation in the Doc dir awaiting merge labels Sep 14, 2018
@1st1 1st1 requested review from willingc and removed request for gpshead and rhettinger September 14, 2018 20:55
@1st1 1st1 added the skip news label Sep 14, 2018
Copy link
Member

@ezio-melotti ezio-melotti left a comment

Choose a reason for hiding this comment

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

There are a few occurrences of asyncio and some classes that have no specific markup. Usually they should either be linkified, or use ``...`` without creating a link.

Copy link
Member

Choose a reason for hiding this comment

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

I haven't seen the rendered version but, based on the content, I think something like 30/70 might look better.
Also using the simple table markup (http://docutils.sourceforge.net/docs/user/rst/quickref.html#tables) might make the source shorter and more readable (same below).

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks pretty good rendered.
screenshot 2018-09-14 14 25 58

Copy link
Contributor

Choose a reason for hiding this comment

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

Adjusting to @ezio-melotti's suggested width would shorten the first entry.

Copy link
Member

Choose a reason for hiding this comment

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

This is pretty much what I expected: it has too much whitespace on the first column and leaves less space for the description (e.g. the first description has to take two lines instead of one).
I think the simple table markup also figures out the column width automatically based on the content (and the list-table probably does the same if you omit the width, unless you want consistency between the tables).

Copy link
Contributor

Choose a reason for hiding this comment

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

Screenshot at 30/70
screenshot 2018-09-14 14 34 21

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the simple table markup also figures out the column width automatically based on the content (and the list-table probably does the same if you omit the width, unless you want consistency between the tables).

No, it doesn't. The table is formatted this way because I configured the columns to be a 50/50 split, which I experimentally found to look the least awful. Unfortunately, while sphinx allows to set percentages for the table columns' widths it doesn't allow to do so for the table itself. Anyways, I'd like to keep the makup as is in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough.
Just out of curiosity, what did you find awful, and what were you trying to achieve (consistent column width between tables, full width table, others)?
(FWIW, of the two screenshots posted by Carol (thanks!), I prefer the 30/70.)

Copy link
Member

Choose a reason for hiding this comment

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

It is a separate exception that doesn't inherit from it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, sadly

@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

Copy link
Contributor

@willingc willingc left a comment

Choose a reason for hiding this comment

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

Fantastic 🐍

Copy link
Contributor

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

Well done!

@1st1
Copy link
Member Author

1st1 commented Sep 14, 2018

There are a few occurrences of asyncio and some classes that have no specific markup. Usually they should either be linkified, or use ... without creating a link.

There are plenty of links to these classes 2-3 lines around those occurrences. Let's not add too many links.

@1st1 1st1 merged commit 7372c3b into python:master Sep 14, 2018
@1st1 1st1 deleted the docsimp5 branch September 14, 2018 22:11
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