-
Notifications
You must be signed in to change notification settings - Fork 17
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
update docs readme #876
update docs readme #876
Conversation
Just noticed this comment from the issue, i'll update wrt it |
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.
Partial review: changes LGTM (pending pandocs -> pandoc).
Will leave final approval to @stephanielee9 or @richrines1
Moved to build_docs and removed the makefile |
if sphinx_paths: | ||
for path in sphinx_paths: | ||
subprocess.run( | ||
f"sphinx-apidoc -f -o source {path} {path}/*_test.py", shell=True, cwd=docs_dir |
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.
we should turn this into a list and remove the shell=True
:
f"sphinx-apidoc -f -o source {path} {path}/*_test.py", shell=True, cwd=docs_dir | |
["sphinx-apidoc", "-f" ,"-o", "source", path, f"{path}/*_test.py"], cwd=docs_dir |
also should we add the "-W"
option so the check fails on warnings? (and ditto for the sphinx-build line below)
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.
That's not a flag for sphinx-apidoc but will add it for the build command
Co-authored-by: richrines1 <85512171+richrines1@users.noreply.github.com>
Co-authored-by: richrines1 <85512171+richrines1@users.noreply.github.com>
Co-authored-by: richrines1 <85512171+richrines1@users.noreply.github.com>
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.
this lgtm % formatting! leaving for @stephanielee9 for final approval though because she's more familiar with the docs infrastructure
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! from what I can tell, the docs still build locally fine with the make file and readthedocs doesn't use it either
Closes #863