-
Notifications
You must be signed in to change notification settings - Fork 40
Adding basic sphinx config to be able to sphinx-build #163
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
Conversation
I don't see the new files being used in the CI, did I miss something? |
88c4c3b
to
09dbf8b
Compare
coming, I just want to see gradually how badly things are breaking. |
…endencies to rely on sphinx-astropy.ext.missing_static
@@ -33,7 +33,7 @@ commands = | |||
pytest {toxinidir}/tests --doctest-plus {posargs} | |||
pytest {toxinidir}/tests --doctest-plus --doctest-rst {posargs} | |||
pytest {toxinidir}/tests --doctest-plus --doctest-rst --text-file-format=tex {posargs} | |||
|
|||
sphinx-build {toxinidir}/tests {toxinidir}/tests/_build/html -W |
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.
I see that it runs fine, yay! But given we cannot see the rendered doc here, we won't catch the problem where it builds without warning but also doesn't show code snippet... 💭
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 were a ton of warnings for the empty code snippets, so I'm confident if there is any issues with the directives we will see warnings if not straight our errors. So I would say let's go ahead, and think about moving stuff to circleCI only if any new sphinx related problem comes up.
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.
Ok, I thought you said there was no warning on RTD when this happened to astroquery but perhaps I misunderstood.
Anyways, I agree we can defer rendering.
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.
Yes, that was my mistake. We were not set up to fail on warnings, and I assumed the "passed" status meant that we are all clean, then locally saw a ton of warnings (not just related to this, but a whole lot of other things crept into the repo in the past many months since we switched CIs...)
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.
Ah, okay... 😬
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.
LGTM but perhaps @saimn wants to have a look as well?
OK, I have to go ahead with this and the release to be able to pin the new version in sphinx-astropy. Thanks @pllim for the review and to point out we need tests for the sphinx extensions. |
I didn't really follow this one but looks good to me if you're happy with it ;) |
This is to be able to run the simplest
sphinx-build tests tests/_build/html
job to render the docs.We only need this to be able to check whether the directives indeed work, not to create a narrative docs, so probably a CI job is enough, no need to drag RTD into the picture.