GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-30607: Use external python-doc-theme by theacodes · Pull Request #2017 · python/cpython

Conversation

@theacodes

@theacodes theacodes commented Jun 9, 2017 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

I have uploaded a build of these docs at http://temp.theadora.io/cpython/index.html

/cc @ncoghlan @brettcannon

https://bugs.python.org/issue30607

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@the-knights-who-say-ni

the-knights-who-say-ni commented Jun 9, 2017

Copy link
Copy Markdown

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 the PSF 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 follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@the-knights-who-say-ni the-knights-who-say-ni added the CLA not signed label Jun 9, 2017
@theacodes

theacodes commented Jun 9, 2017

Copy link
Copy Markdown
Contributor Author

(I just signed the CLA, I'll nag the bot tomorrow when hopefully it'll have been reviewed.)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@brettcannon

brettcannon commented Jun 9, 2017

Copy link
Copy Markdown
Member

Just an FYI @jonparrott I just added the CLA bot to the theme repo.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
brettcannon
brettcannon reviewed Jun 9, 2017
View reviewed changes
Comment thread .travis.yml Outdated
# (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

@brettcannon brettcannon Jun 9, 2017

Copy link
Copy Markdown
Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ned-deily ned-deily Jun 9, 2017

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@theacodes theacodes Jun 9, 2017

Copy link
Copy Markdown
Contributor Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@zware zware Jun 9, 2017

Copy link
Copy Markdown
Member

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 See https://github.com/python/docsbuild-scripts

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@theacodes theacodes Jun 9, 2017

Copy link
Copy Markdown
Contributor Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@theacodes theacodes Jun 9, 2017

Copy link
Copy Markdown
Contributor Author

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ncoghlan ncoghlan Jun 10, 2017

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@theacodes theacodes Jun 13, 2017

Copy link
Copy Markdown
Contributor Author

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. :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ned-deily ned-deily self-assigned this Jun 9, 2017
@the-knights-who-say-ni the-knights-who-say-ni added CLA signed and removed CLA not signed labels Jun 13, 2017
theacodes pushed a commit to theacodes/docsbuild-scripts that referenced this pull request Jun 13, 2017
Jon Wayne Parrott
Add common docs theme as a dependency
487b044 
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
@theacodes theacodes mentioned this pull request Jun 13, 2017
Add common docs theme as a dependency python/docsbuild-scripts#12
Merged
@theacodes

theacodes commented Jun 13, 2017

Copy link
Copy Markdown
Contributor Author

CLA signed. :)

I've also created python/docsbuild-scripts#12
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Mariatta Mariatta added needs rebase and removed needs rebase labels Oct 9, 2017
@theacodes

theacodes commented Jan 19, 2018

Copy link
Copy Markdown
Contributor Author

I'm happy to rebase this and keep it current, but this kind of died of lack of attention.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@zware

zware commented Jan 19, 2018

Copy link
Copy Markdown
Member

With python/docsbuild-scripts#12 merged, I don't think there's anything blocking this now.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@theacodes

theacodes commented Jan 19, 2018

Copy link
Copy Markdown
Contributor Author

Thanks @zware, I'll attempt to rebase this (& reconcile any theme changes) over the weekend.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ned-deily
ned-deily requested changes Jan 22, 2018
View reviewed changes

@ned-deily ned-deily left a comment

Copy link
Copy Markdown
Member

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 on python/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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot

bedevere-bot commented Jan 22, 2018

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting changes label Jan 22, 2018
Jon Wayne Parrott
bpo-30607: Use external python-doc-theme
9d0b0c1
@theacodes
theacodes force-pushed the use-common-docs-theme branch from 2f2e0f1 to 9d0b0c1 Compare January 25, 2018 21:54
@theacodes

theacodes commented Jan 25, 2018

Copy link
Copy Markdown
Contributor Author

Alright, this is rebased and ready to review & merge. Thanks @ned-deily and @zware.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Jon Wayne Parrott
Add news entry
79d1985
@ncoghlan

ncoghlan commented Jan 27, 2018

Copy link
Copy Markdown
Contributor

Note that https://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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@theacodes

theacodes commented Feb 2, 2018

Copy link
Copy Markdown
Contributor Author

Gentle ping on this - I'd like to get it merged & hopefully do so before the branch gets stale. :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ned-deily
ned-deily requested a review from JulienPalard February 9, 2018 04:40
@ned-deily ned-deily removed their assignment Feb 9, 2018
ned-deily
ned-deily approved these changes Feb 9, 2018
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting changes labels Feb 9, 2018
@ned-deily

ned-deily commented Feb 9, 2018

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@theacodes

theacodes commented Feb 9, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

One other thing - the daily docs build system may need a change to install python-docs-theme.

@ned-deily is that what I did in python/docsbuild-scripts#12?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ned-deily

ned-deily commented Feb 10, 2018

Copy link
Copy Markdown
Member

is that what I did in python/docsbuild-scripts#12?

Ah, yes. That should be updated to require the new package rather than getting from Github.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
theacodes pushed a commit to theacodes/docsbuild-scripts that referenced this pull request Feb 12, 2018
Jon Wayne Parrott
Pull common docs theme from PyPI
f9b1112 
Context: python/cpython#2017 (comment)
@theacodes theacodes mentioned this pull request Feb 12, 2018
Pull common docs theme from PyPI python/docsbuild-scripts#41
Merged
@theacodes

theacodes commented Feb 12, 2018

Copy link
Copy Markdown
Contributor Author

@ned-deily PR created: python/docsbuild-scripts#41 (I don't think that blocks this)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
JulienPalard pushed a commit to python/docsbuild-scripts that referenced this pull request Feb 12, 2018
Jon Wayne Parrott @JulienPalard
Pull common docs theme from PyPI
bd00712 
Context: python/cpython#2017 (comment)
@JulienPalard

JulienPalard commented Feb 12, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Merged python/docsbuild-scripts#41 after testing it locally.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@theacodes

theacodes commented Feb 12, 2018

Copy link
Copy Markdown
Contributor Author

Thanks @JulienPalard. Anything else we need to do to get this merged? :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Mariatta added a commit to python/devguide that referenced this pull request Mar 1, 2018
@Mariatta
Reference python-docs-theme when building docs without make
3b4014f 
Only merge once python/cpython#2017 has been merged.
@Mariatta Mariatta mentioned this pull request Mar 1, 2018
[don't merge yet] Reference python-docs-theme when building docs without make python/devguide#333
Closed
@Mariatta

Mariatta commented Mar 1, 2018

Copy link
Copy Markdown
Member

Tested locally, things looking good.
We can merge devguide PR python/devguide#333 once this has been merged.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Mariatta
Mariatta approved these changes Mar 1, 2018
View reviewed changes
@theacodes

theacodes commented Mar 1, 2018

Copy link
Copy Markdown
Contributor Author

Thanks, Mariatta!

Does anything else need to be done here? Let's merge?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ned-deily
ned-deily merged commit bf63e8d into python:master Mar 1, 2018
@bedevere-bot bedevere-bot removed the awaiting merge label Mar 1, 2018
@ned-deily

ned-deily commented Mar 1, 2018

Copy link
Copy Markdown
Member

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.)
Mariatta reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@theacodes

theacodes commented Mar 1, 2018 via email

Copy link
Copy Markdown
Contributor Author
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> .
Mariatta reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brettcannon brettcannon brettcannon left review comments
@ncoghlan ncoghlan ncoghlan left review comments
@zware zware zware left review comments
@ned-deily ned-deily ned-deily approved these changes
@Mariatta Mariatta Mariatta approved these changes
@JulienPalard JulienPalard Awaiting requested review from JulienPalard

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@theacodes @the-knights-who-say-ni @brettcannon @zware @bedevere-bot @ncoghlan @ned-deily @JulienPalard @Mariatta
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.