-
Notifications
You must be signed in to change notification settings - Fork 12
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
[#15] Integration for Sphinx documentation build #126
Conversation
9fe65b7
to
935d437
Compare
I think that improved aesthetics can be left to future PRs. Automatic build and deployment is, in my opinion, more important. |
b087183
to
4bd1253
Compare
I rebased to master and made necessary changes.Here's the deployed current docs version so it's easier to view: The MR includes the workflow to push the commit onto the gh-pages and trigger the build & deployment. I tested it on our repository and it worked. It is quite redundant to wiki pages so let me know if you think I should merge that somehow. I'm aware few things can be done for our docs to look more neat; like unifying the text or documenting the The "Winter cleaning" did wonders for our API hierarchy. It's so much more readable now. Please let me know what you think and if you've got any suggestions for me. |
All docstings need to comply with numpydoc style. This is necessary for API to be generated properly.
I think that building (or verifying in some other way) the docs is needed on pull requests. Otherwise, we might get a pull request which passes checks, but then fails building docs on master. |
That's exactly it, I did in my venv:
And everything went fine. |
Okay, so I think I managed to separate the build from the deployment of github pages so that the pages are only deployed when on master and the documentation build occurs either on master or any PR with master as a target branch. I haven't managed as to why |
As per the inaccurate arguments to class constructor. We can do a quick fix like this: What I did here is I separated the class parameters and added I found that there was issue open for this exact bug in autodoc but it supposedly got fixed in 3.4 release (we use 5.1): Let me know what you think. |
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.
For me it looks great. Lets merge and eventually fix on production ;)
Co-authored-by: lekcyjna123 <34948061+lekcyjna123@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.
LGTM. I like the sphinx config, it is quite similar to what I have worked with (see https://docs.pwntools.com if curious), except this one uses MyST.
The GH-MD rendering is a bit of a pain. I wonder if this can be worked around using MyST config options, but that might be out of this PR's scope.
@@ -14,7 +14,7 @@ Its main tasks are: | |||
|
|||
## Schema | |||
|
|||
```mermaid | |||
```{mermaid} |
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.
One downside I can see is that GitHub no longer renders the mermaid graphs in-tree (it once did).
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've been searching for a while for a hack for this case (I tried the one that worked for people for Gitlab & Sphinx (executablebooks/MyST-Parser#366) but it seems that Github is picky and cannot ignore the ::
after the directive) but I haven't got much luck finding solution. (I mean I did find a hack somewhere but this would require modifying myst paser internal code.
The thing is Github's support for mermaid is very fresh.
Actually, the issue just opened on sphinxcontrib-mermaid
on our problem:
mgaitan/sphinxcontrib-mermaid#99
Best what I can propose as of now, waiting for progress of above-mentioned issue, render those two particular mermaid graphs and just store them as pngs for consistency.
Indeed, there are problems with rendering in GitHub. I wonder how much of an issue is this. It certainly interferes with the way we (ab)used the GitHub's wiki for deployment - but with this PR, the wiki deployment becomes redundant, and maybe it should be removed. |
This way there's no need for custom anchors that show with usual MD rendering.
5e6749c
to
bdff167
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.
I'm for merging this ASAP, what remains is making the linter happy. I've also suggested some small docstring typo fixes, which we can do here as there is a lot of docstring fixes here anyway.
Co-authored-by: Marek Materzok <tilk@tilk.eu>
Co-authored-by: Marek Materzok <tilk@tilk.eu>
Co-authored-by: Marek Materzok <tilk@tilk.eu>
Draft PR adding the integration for auto-generating API docs from docstrings and merging it into self-written documentation.
It also includes the changes in said docstrings to be up to
numpydoc
style standards to then be generated without errors/warning in the result document.As for what's need for this PR to be mergable:
There are some aesthetic things that could be improved (i.e. the way
enum
documentations are generated).There need to be a job added to build and deploy documentation, as we already have Github Pages set up.