-
Notifications
You must be signed in to change notification settings - Fork 11
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
docs: use quantinuum-sphinx theme #120
Conversation
d3b421b
to
aedb662
Compare
thanks for sorting this out, I should be able to finish this off today |
7432592
to
0aea766
Compare
0aea766
to
aab2c5c
Compare
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.
Looking great!
Would you mind checking the warning from the build. I see a warning
WARNING: html_static_path entry 'quantinuum-sphinx/_static' does not exist
WARNING: favicon file 'quantinuum-sphinx/_static/assets/quantinuum_favicon.svg' does not exist
Could those be fixed?
I've also added a README in the Would you recommend using poetry or pip in local build and github workflows? |
- name: Save documentation | ||
uses: actions/upload-artifact@v4 | ||
with: | ||
name: docs_html |
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.
Slightly worried I've got this line and the one below wrong? Maybe doesn't matter so much.
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 don't think it matters hugely, but I think only the build itself really needs uploading here. Would you mind making that change?
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.
Fixed now
Warnings now fixed, hadn't checked out the submodule in C.I. properly. These warnings were actually quite serious. I've added a |
Thanks, looking good! I don't really recommend using either in particular, but I'm happy to stick with pip in this readme. |
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.
Just this last small comment, then it's good to go!
docs/README.md
Outdated
```shell | ||
python -m venv ".venv" | ||
source .venv/bin/activate | ||
pip install -e .[docs] | ||
``` |
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 think we could remove the reference to venv and just allow the user to use whichever means they prefer.
You could also mention at this point that poetry is supported.
I've created this new branch with your changes on top of main. Let me know if this is actually helpful.