Skip to content

bpo-11233: Create availability directive for documentation #9692

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

Merged
merged 8 commits into from
Oct 12, 2018

Conversation

csabella
Copy link
Contributor

@csabella csabella commented Oct 4, 2018

Original patch by Georg Brandl.

https://bugs.python.org/issue11233

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

I like the general approach, but there are a few issues:

  • "make html" generates 3 warnings

/home/vstinner/prog/python/master/Doc/library/os.rst:1111: WARNING: Explicit markup ends without a blank line; unexpected unindent.
/home/vstinner/prog/python/master/Doc/library/os.rst:1179: WARNING: Explicit markup ends without a blank line; unexpected unindent.
/home/vstinner/prog/python/master/Doc/library/os.rst:3555: WARNING: Explicit markup ends without a blank line; unexpected unindent.

  • "Availability: Unix." is used outside os.rst: asyncio, signal, time, etc. documentation.

  • Because the availability is used in multiple files, maybe the definition of availability should be described somewhere else? Sorry I have no better proposal, so maybe allos.rst is fine.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@csabella csabella requested review from 1st1, asvetlov, gpshead and a team as code owners October 11, 2018 11:33
@csabella
Copy link
Contributor Author

@vstinner

  • I fixed the 'make html' warnings. Sorry that I missed those the first time.
  • Based on the comment about other sources, I also added the availability directive to the other sources. I did it in its own commit so it could be reversed if it's too much.
  • As far as where to store the definition of availability, I looked for a global 'notes'-type page to add it to, but didn't find one. The closest concept that I could find is maybe 'CPython Implementation Detail', but that doesn't have a note defining it. For now, I've moved it to the library intro page. It doesn't flow well with the opening paragraphs, so maybe it should be its own page at the end of the TOC?

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

Hum, I would prefer that the macro doesn't add a trailing dot. I prefer to give more freedom to the user of the macro to decide how to render the text.

@vstinner vstinner merged commit 2d6097d into python:master Oct 12, 2018
@vstinner
Copy link
Member

Thanks @csabella! I checked the full PR and it seems like all "Availability" lines now ends with a dot :-) It's more consistent. For the exact location of the definition of Availability, if someone complains, we can now easily change it, since it has a reST label. The whole PR LGTM, I merged it.

I don't want it to be backported. It seems too intrusive to backport it.

@1st1
Copy link
Member

1st1 commented Oct 12, 2018

a post-commit LGTM from me.

@1st1
Copy link
Member

1st1 commented Oct 12, 2018

BTW, I think this PR should be backported to 3.7, otherwise we might have unnecessary conflicts when backporting docs updates to 3.7.

@vstinner
Copy link
Member

BTW, I think this PR should be backported to 3.7, otherwise we might have unnecessary conflicts when backporting docs updates to 3.7.

If you want to avoid conflicts, in that case, should we also apply it to 3.6 and 2.7?

@1st1
Copy link
Member

1st1 commented Oct 12, 2018

We rarely backport doc changes to 2.7 and 3.6. IMO 3.7 would be good enough.

@csabella csabella deleted the bpo11233 branch October 12, 2018 21:13
@csabella
Copy link
Contributor Author

Do you want me to cherry pick to 3.7 or is Miss Islington able to do it post-merge?

@miss-islington
Copy link
Contributor

Thanks @csabella for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry, @csabella and @vstinner, I could not cleanly backport this to 3.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 2d6097d027e0dd3debbabc702aa9c98d94ba32a3 3.7

@1st1
Copy link
Member

1st1 commented Oct 12, 2018

Do you want me to cherry pick to 3.7 or is Miss Islington able to do it post-merge?

Apparently she can't :( Could you please open a PR for 3.7 branch?

csabella added a commit to csabella/cpython that referenced this pull request Oct 12, 2018
…honGH-9692)

 Replace "Availability: xxx" with ".. availability:: xxx" in the doc.
 Original patch by Georg Brandl.

 Co-Authored-By: Georg Brandl <[email protected]>
 (cherry picked from commit 2d6097d)

 Co-authored-by: Cheryl Sabella <[email protected]>
@bedevere-bot
Copy link

GH-9830 is a backport of this pull request to the 3.7 branch.

vstinner pushed a commit that referenced this pull request Oct 15, 2018
…9692) (GH-9830)

Replace "Availability: xxx" with ".. availability:: xxx" in the doc.
 Original patch by Georg Brandl.

 Co-Authored-By: Georg Brandl <[email protected]>
 (cherry picked from commit 2d6097d)

 Co-authored-by: Cheryl Sabella <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants