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

Fix documentation compilation #207

Merged
merged 23 commits into from
Sep 4, 2024

Conversation

thierry-martinez
Copy link
Contributor

@shinich1 noticed that documentation compilation was broken (see comment). This pull request fixes examples, cross-references and doc-comments to fix various errors. Another commit adds a doc workflow for CI validation of the documentation (as suggested by @EarlMilktea, #198 (comment)).

I used sphinx-build --nitpicky first to catch more errors, but I didn't manage to make it resolve references to nx_reportviews.EdgeView, so I removed the option.

@shinich1
Copy link
Contributor

great PR, thanks!

@EarlMilktea
Copy link
Contributor

EarlMilktea commented Aug 30, 2024

@thierry-martinez

Read the Docs has the official CI framework: https://docs.readthedocs.io/en/stable/integrations.html (you can see the deployment/errors via GUI like this: https://readthedocs.org/projects/cupy/builds/25317461/ )
Why not use it?

@thierry-martinez
Copy link
Contributor Author

We could configure Read the Docs CI, but the integration with GitHub seems to require admin rights on both sides (GitHub and Read the Docs). On the other hand, I was able to add the doc workflow without needing admin rights on either platform.

What are the benefits of using Read the Docs CI instead of GitHub Actions?

Perhaps we could merge this PR first since it fixes a number of errors and the CI check with GitHub Actions is working properly. We could then propose using Read the Docs CI in a separate issue or PR, where someone with admin rights can handle the setup. What do you think?

@EarlMilktea
Copy link
Contributor

Either looks good to me.

@EarlMilktea
Copy link
Contributor

Related to package documentation/comment, let me suggest an idea: why not use Ruff rule D for better documentation styling?
We may need to turn off D10 for now to allow undocumented public items.

@shinich1
Copy link
Contributor

Perhaps we could merge this PR first since it fixes a number of errors and the CI check with GitHub Actions is working properly. We could then propose using Read the Docs CI in a separate issue or PR, where someone with admin rights can handle the setup. What do you think?

I think we can merge this PR after adding D rule as @EarlMilktea suggested. We should explore github pages which might integrate better with github actions.

@EarlMilktea
Copy link
Contributor

Memo: May need to completely ignore rule D for examples/*.py.

@thierry-martinez
Copy link
Contributor Author

I turned on the D rule in fd1c57c, and add/fixed the docstrings. D rule is ignored in benchmarks, examples and tests.

@EarlMilktea
Copy link
Contributor

EarlMilktea commented Aug 31, 2024

@thierry-martinez

How thorough it is! Excellent!
Let me suggest some additional fixes on https://github.com/TeamGraphix/graphix/tree/fix-doc-ss .

graphix/channels.py Outdated Show resolved Hide resolved
docs/source/data.rst Outdated Show resolved Hide resolved
graphix/channels.py Outdated Show resolved Hide resolved
graphix/command.py Outdated Show resolved Hide resolved
graphix/extraction.py Outdated Show resolved Hide resolved
@EarlMilktea
Copy link
Contributor

MEMO: Opened #208 to speed up CI checks.

thierry-martinez and others added 5 commits August 31, 2024 21:43
Co-authored-by: Shinichi Sunami <[email protected]>
Co-authored-by: Shinichi Sunami <[email protected]>
Co-authored-by: Shinichi Sunami <[email protected]>
Co-authored-by: Shinichi Sunami <[email protected]>
Co-authored-by: Shinichi Sunami <[email protected]>
@thierry-martinez
Copy link
Contributor Author

If there's anything else I should address before merging, please let me know!

Copy link
Contributor

@EarlMilktea EarlMilktea left a comment

Choose a reason for hiding this comment

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

Great! Only some minor suggestions.

.github/workflows/doc.yml Show resolved Hide resolved
.github/workflows/doc.yml Show resolved Hide resolved
docs/source/conf.py Outdated Show resolved Hide resolved
docs/source/conf.py Outdated Show resolved Hide resolved
@shinich1
Copy link
Contributor

shinich1 commented Sep 4, 2024

@thierry-martinez LGTM! please squash and merge after resolving conflict, if you think it's ready?

@thierry-martinez thierry-martinez merged commit ad18270 into TeamGraphix:master Sep 4, 2024
19 checks passed
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.

3 participants