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

Fix indentation in contextlib documentation to three spaces #94361

Conversation

JustAnotherArchivist
Copy link
Contributor

There was a stray space at the beginning of some lines in the async @timeit example.

As this is a trivial typo fix, no issue exists, and a news entry shouldn't be necessary.

@bedevere-bot bedevere-bot added awaiting review docs Documentation in the Doc dir labels Jun 28, 2022
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 would be more consistent to increase the indentation in other part of the example.

@JustAnotherArchivist
Copy link
Contributor Author

Oh, good point, I didn't compare it to the other code blocks in that file. Yeah, I suppose it would be slightly more consistent, although I see two, three, four, and five spaces of indentation relative to the preceding paragraphs... It seems like three is the most common though, and that's what the dev guide says as well (three spaces in reST, 4 spaces inside the Python blocks, but the indentation of the code blocks would be part of reST syntax). I'll normalise the entire document to use three for consistency.

@JustAnotherArchivist JustAnotherArchivist changed the title Fix indentation depth typo in contextlib.asynccontextmanager documentation example Fix indentation in contextlib documentation to three spaces Jun 28, 2022
@merwok
Copy link
Member

merwok commented Jul 1, 2022

I don’t think the churn is worth it. This will not change the output, nor help developers.

@serhiy-storchaka
Copy link
Member

I mostly agree with @merwok -- the churn is too large, and in most cases it does not affect the output. But in two cases, in the original code example and in the versionchanged directive for asynccontextmanager, incorrect indentation affects the output. It is a bug which should be fixed. I may miss other such cases, please re-check me.

@slateny
Copy link
Contributor

slateny commented Sep 25, 2022

@JustAnotherArchivist Are you still interested in combing through the docs for whitespace / visible formatting changes? I think the important ones listed here are the original stray space and also some three-space indentation in code examples. Another possible change is to remove the >>> from code examples as it doesn't help much with copying:

image

@merwok
Copy link
Member

merwok commented Sep 26, 2022

Another possible change is to remove the >>> from code examples as it doesn't help much with copying:

I think that’s been addressed quite some time ago: there is now a formatting that adds buttons to toggle prompts on/off, and easily copy the code block.

@slateny
Copy link
Contributor

slateny commented Sep 26, 2022

I think that’s been addressed quite some time ago: there is now a formatting that adds buttons to toggle prompts on/off, and easily copy the code block.

Not 100% on how it works, but in the case above it doesn't have this button in the top right:
image

I'm guessing that all of the code example would need the >>>/... for the button to be generated, otherwise it thinks it's a regular code block.

@slateny
Copy link
Contributor

slateny commented Oct 2, 2022

I'll start on a new PR here then and close this when that one's open - let me know if you'd still like to continue this one.

@slateny
Copy link
Contributor

slateny commented Oct 9, 2022

See #98111

@slateny slateny closed this Oct 9, 2022
@JustAnotherArchivist
Copy link
Contributor Author

Apologies, I hadn't seen the notification emails for this for some reason. I disagree with the 'churn' argument, but that's a bit late now. Thanks for the fix, @slateny!

@slateny
Copy link
Contributor

slateny commented Dec 8, 2022

No worries, thanks for the update 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants