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

Use theme release 0.5.0rc1 for docs #7037

Merged
merged 8 commits into from Jun 16, 2020
Merged

Use theme release 0.5.0rc1 for docs #7037

merged 8 commits into from Jun 16, 2020

Conversation

agjohnson
Copy link
Contributor

@agjohnson agjohnson commented May 6, 2020

Also upgrade Sphinx to 3+

Copy link
Member

@ericholscher ericholscher left a comment

Makes sense w/ passing tests

@agjohnson
Copy link
Contributor Author

agjohnson commented May 6, 2020

The issue was a duplicate definition in requirements/pip.txt for Sphinx. I upgraded there but have no clue if we can use the version in our code.

If this works and we can't use sphinx 3.0.3 in code, then i'll make a mess of our requirements files to install Sphinx 3 after the docs requirements file.

@ericholscher
Copy link
Member

ericholscher commented May 7, 2020

I'm getting a flash of a different font when I first load the docs, not sure whats going on there.

@agjohnson
Copy link
Contributor Author

agjohnson commented May 7, 2020

Opened up readthedocs/sphinx_rtd_theme#913 to address this. The behavior seems to be prod docs produce invisible text while loading and 0.5.0rc1 uses the old behavior for unstyled text.

* Upgrade Sphinx to 3.0.4
* Redo some of our doc requirements files to make them less complicated
@agjohnson
Copy link
Contributor Author

agjohnson commented Jun 5, 2020

I've rebased and i'm testing changes that separate docs dependencies from application dependencies. This is to work around the inability to upgrade docs Sphinx independent of the application Sphinx dependency.

I've bumped to 0.5.0rc2 to fix some of the small issues with 0.5.0rc1

@agjohnson agjohnson requested a review from ericholscher Jun 5, 2020
@ericholscher
Copy link
Member

ericholscher commented Jun 15, 2020

Believe we should ship this now right?

@@ -48,7 +48,7 @@ commands =

[testenv:docs-lint]
description = run linter (rstcheck) to ensure there aren't errors on our docs
deps = -r{toxinidir}/requirements/docs-lint.txt
deps = -r{toxinidir}/requirements/docs.txt
Copy link
Member

@ericholscher ericholscher Jun 15, 2020

Choose a reason for hiding this comment

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

Don't we still need rstcheck?

Copy link
Member

@ericholscher ericholscher Jun 15, 2020

Choose a reason for hiding this comment

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

Looks like it's already in docs.txt -- so we should remove the docs-lint file.

Copy link
Member

@ericholscher ericholscher Jun 15, 2020

Choose a reason for hiding this comment

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

(removed in a commit)

@ericholscher
Copy link
Member

ericholscher commented Jun 16, 2020

@agjohnson looks like we have conflicting version issues, so I think that's why we had local-docs-build. I can't figure out a way to make it work :/

@ericholscher
Copy link
Member

ericholscher commented Jun 16, 2020

Actually -- I figured it out by hacking the install into the test command 👍

@ericholscher
Copy link
Member

ericholscher commented Jun 16, 2020

Shipping!

@ericholscher ericholscher merged commit e5e5be3 into master Jun 16, 2020
2 checks passed
@ericholscher ericholscher deleted the agj/theme-0.5.0rc1 branch Jun 16, 2020
@agjohnson
Copy link
Contributor Author

agjohnson commented Jun 16, 2020

looks like we have conflicting version issues, so I think that's why we had local-docs-build

Yeah, I separated docs from pip requirements because of this. We could add back this line eventually:
https://github.com/readthedocs/readthedocs.org/pull/7037/files#diff-c7e6b68f10ee36406c7a76f4c4d988d6L1

But i'm not convinced that's the best pattern. I think your patch makes sense.

I believe we can also do:

deps = 
    -r{toxinidir}/requirements/pip.txt
    -r{toxinidir}/requirements/docs.txt

@ericholscher
Copy link
Member

ericholscher commented Jun 16, 2020

Yea, I tried that but it runs them both at the same time and conflicts, which is why I had to hack it.

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.

None yet

2 participants