Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.1k
bpo-30607: Use external python-doc-theme#2017
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
the-knights-who-say-ni commentedJun 9, 2017
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed thePSF contributor agreement (CLA). Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please followthe steps outlined in the CPython devguide to rectify this issue. Thanks again to your contribution and we look forward to looking at it! |
(I just signed the CLA, I'll nag the bot tomorrow when hopefully it'll have been reviewed.) |
Just an FYI@jonparrott I just added the CLA bot to the theme repo. |
.travis.yml Outdated
| @@ -37,7 +37,8 @@ matrix: | |||
| - cd Doc | |||
| # Sphinx is pinned so that new versions that introduce new warnings won't suddenly cause build failures. | |||
| # (Updating the version is fine as long as no warnings are raised by doing so.) | |||
| - python -m pip install sphinx~=1.6.1 | |||
| # The theme used by the docs is stored seperately, so we need to install that as well. | |||
| - python -m pip install sphinx~=1.6.1 git+https://github.com/python/python-docs-theme.git#egg=python-docs-theme | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Will there need to be a change made to the docs.python.org build step for the docs to pull the theme in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Also need to review how this change might affect installer builds and release builds which build copies of the docs. Assigned myself to review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@brettcannon probably, but I don't know where that build step is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@zware yes, we'll just need to add the theme package to the requirements.txt there. I'll send a PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actually before I do that-@ncoghlan you mentioned that you didn't think we should publish the theme to PyPI, but it's seeming like it's going to be a lot more convenient to do that if we're gonna use it here. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@jonparrott The main problem I see with it is that PyPI doesn't have great "teams" support, so it adds an awkward extra set of permissions to manage in an already awkward update process.
Whereas if we're installing the theme directly from git, then the CPython docs maintainers should already have all the access they need to update it - they'll just be modifying a different repo, rather than having to worry about also tagging that repo and making a new PyPI release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@ncoghlan great point. I like it, I just wanted to understand why. I can add this to the comments here and in the build scripts. :)
Additionally update all existing dependencies in requirements.txt, as the common theme requires sphinx >= 1.6.0.Context:*python/cpython#2017*https://bugs.python.org/issue30607
CLA signed. :) I've also createdpython/docsbuild-scripts#12 |
I'm happy to rebase this and keep it current, but this kind of died of lack of attention. |
Withpython/docsbuild-scripts#12 merged, I don't think there's anything blocking this now. |
Thanks@zware, I'll attempt to rebase this (& reconcile any theme changes) over the weekend. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
See my comments onpython/python-docs-theme#5 about unnecessarily adding a docs build dependency on Git. Also, the PR needs to be rebased and retested to accommodate the changes made to support multiple languages. At that time, the full docs build should be tested ("make dist"). The step needed to install the theme package (presumably now from PyPI rather than from Github) should be added to the Makefile's "venv" recipe ("make venv") which currently installs the complete Docs build toolchain (now blurb, sphinx, and dependencies).
bedevere-bot commentedJan 22, 2018
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 |
2f2e0f1 to9d0b0c1CompareAlright, this is rebased and ready to review & merge. Thanks@ned-deily and@zware. |
Note thathttps://pypi.python.org/pypi/python-docs-theme/ is now 0.0.1, and there's only one version, so we can postpone questions of whether or not we want to pin versions on different CPython branches or not. |
Gentle ping on this - I'd like to get it merged & hopefully do so before the branch gets stale. :) |
Thanks for addressing my review comment; distributing via PyPI looks much cleaner to me. @JulienPalard if you have the time and interest, I would appreciate your review of this, especially if there any implications for the translated docs. Otherwise, we can deal with fixes during the 3.8 feature dev cycle (this change is not appropriate for 3.7 or earlier systems). One other thing - the daily docs build system may need a change to install python-docs-theme. |
theacodes commentedFeb 9, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@ned-deily is that what I did inpython/docsbuild-scripts#12? |
Ah, yes. That should be updated to require the new package rather than getting from Github. |
@ned-deily PR created:python/docsbuild-scripts#41 (I don't think that blocks this) |
JulienPalard commentedFeb 12, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Mergedpython/docsbuild-scripts#41 after testing it locally. |
Thanks@JulienPalard. Anything else we need to do to get this merged? :) |
Only merge oncepython/cpython#2017 has been merged.
Tested locally, things looking good. |
Thanks, Mariatta! Does anything else need to be done here? Let's merge? |
Thanks Jon and everyone else who worked on this. Let's see how it works for 3.8. (I don't think this should be backported.) |
Yay! …On Thu, Mar 1, 2018, 1:06 PM Ned Deily ***@***.***> wrote: Thanks Jon and everyone else who worked on this. Let's see how it works for 3.8. (I don't think this should be backported.) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2017 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAPUc0L6MqoS7GVIS7ANP1mC-5e9NtNiks5taGLGgaJpZM4N04vU> . |
Uh oh!
There was an error while loading.Please reload this page.
I have uploaded a build of these docs athttp://temp.theadora.io/cpython/index.html
/cc@ncoghlan@brettcannon
https://bugs.python.org/issue30607