documentation overhaul #816
Conversation
|
This is clearly a huge amount of work. Thank you for doing it. It is a lot to put in a single PR. Could there be room to break this out a bit? I could try to do that for you if you like and leave you as the author of the commits? I can understand why some of it is grouped together but it seems like certain parts could be separate? The mypy changes? The theme change? The copy button? I realise those are small parts though. I would also consider it desirable to have more information in the commit messages. It is always good to have the thought process and reasoning recorded along with change. I appreciate you have included that in the PR message but I generally consider git commit messages to be longer lasting than pull requests. I would also recommend using I'm sorry for those slightly negative points on what is clearly a much overdue and massively needed chunk of work. I have long gotten used to the reams of warnings/errors generated by the documentation but it is good to work through them and silence them where possible. We might have some under specified dependencies though. When I run: in the root of the project. It fails with: Do you have any thoughts on why that might be? |
|
Well, I could break out the theme changes easy enough. I've never had to retroactively change commit messages before, so that's a learning experience I look forward to. As to the error message, I've not seen that before, and don't know where to start other than figure out what lib is using pyparsing (my guess it is related to pygments internals). The complete error message should say where the full traceback is (as a log file in a tmp folder). I would first cut the makefile (which may be hiding some relevant error info) out of the equation by running |
|
It gets a bit further now! It seems to fail with: That might be due to my version of doxygen though which is |
|
see #811 . We would only require users install doxygen 1.9.2+ if documenting c++20 concepts. Beware 1.9.2I've had problems in the past concerning mismatched XML tags when running breathe with doxygen v1.9.2 See #782 for hints about installing doxygen from source forge binary distributed archives. Personally, I've grown to weary of using |
|
Ah, ok. So concept support has been in for a while and requires 1.9.2+? So we're already kind of forced onto that for our docs as we include concept examples in the final docs? Seems like a good call to test against older versions if we can somehow handle that. Thanks for your multi-doxygen version suggestion. |
Yes and yes. In the future it might be better to build the docs for each test (as a separate CI workflow using a matrix) in the examples folder rather than displaying the working results in the docs. This would also help avoid duplicate IDs declared in reference docs and test pages. |
|
Sensible suggestion, yeah. My set up is now failing with: I realise graphviz support was also merged recently. I do have dot on my path. And these dot files seem to have been generated: Do you know what might have gone wrong? Sorry that I'm just leaning on you for this stuff. Ideally I would know what is happening. |
|
Its likely a path problem. Doxygen's XML output names dotfiles (the arg passed to |
|
After I get this PR broken up, I'll look into what can be done to battle-harden the dotfile paths (python's |
|
I get some form of that error whether I run it with |
|
Well, I rolled back the theme changes and pygments typing changes (will submit those as separate PRs shortly), but I can't see an easy way to break out any more changes as they're all tightly related. I found that gitkraken UI allows easily renaming old commits (& their descriptions), so I'll try to do some of that later today. I tried to reproduce the dotfile problem, but I couldn't. It'd be great to have the full traceback for the error you're reporting. What I did to try to reproduce itthat worked, so... that worked, so try to fail on purpose that worked?! ok try to fail at repo root that worked too?! Its worth mentioning that I'm running these dev ops in Windows using WSL. If I run doxygen in Windows and sphinx in Linux (or vice versa), then I get a path-related error about the dotfile. I understand 88 files is a big ask for review. FWIW, I just got a review request for a PR with 246 file changes (some are just copyright year updates). So, I feel your pain. Please, take your time. |
|
Thank you for reverting some of the changes. I might squash the branch down to a few commits tomorrow if that's ok. I also like talking about git and doing history editing so if you'd be up for a video call or something we could chat about the joys of |
|
I'm used to squashing all commits in a PR, so yeah I'm good with that.
🤣 that textual sarcasm right? I tend to avoid using git CLI. I know its a disadvantage, but I always end up breaking stuff when I try using the CLI. About the only thing I've learned to do well with CLI is submodules. |
|
ok I modified the description of this commit using gitkraken. I didn't realize that pushing the change would act like a rebase. What's the best course of action for multiple commits? (aside from committing more often in the future)🤦🏼 |
A little but I really appreciate the flexibility that
Git does have a pretty terrible user experience. It is one of the few tools where I think it is best to learn about the underlying data structure of the graph in order to better understand what you can do with it. Bit of a shame that that is necessary but fortunately the underlying structure isn't too complex. |
By adjusting either the restructured text or code/doxygen setup as needed. Including some renaming of C/C++ symbols for uniqueness so that we don't have duplicate IDs throughout the docs. Other parts use no-link to avoid generating unused or duplicate IDs. Some other parts are commented out in the restructured text so that we don't render them any more. For example, the mux vdhl stuff in the doxygen test suite. We've include the lists of warnings that doxygen generates but we can get rid of those at some point.
- tell sphinx to treat warnings as errors (replacing nit-picky mode) - fix awkward word choice in a a doc - RTD will use conda forge to get updated docygen executable. GH CI will get doxygen from SourceForge archives binary executable - document the entire namespace instead individual members of the typeedf example test - use a different member as the `extern` member test (for reason related to undefined types in member's parameters) - define empty structures to use as a reference for a type in a parameter/return value of a documented function - add comments to explain what the empty structures are for - remove unnecessary inheritence from "image" example test
We use explicitly language-typed code blocks much more than before to have better syntax highlighting. We use links where possible instead of plain text references to directives and config variables. We reformat re-indent some of the text blocks as needed.
We don't need a record of them.
|
I've force-pushed a new set of commits. They are basically the main ones you made. You remain the author though git stores me as the committer. I've pushed the original state of your branch as |
If we're not supporting it (and we probably shouldn't) then there is no need to keep it around in the repository.
If there appear elsewhere in the docs then that is all that matters. We don't need to keep commented out content about them.
If DOTFILE_DIRS tag is specified, then doxygen 1.9.3 will copy the dot file into the XML output folder and use a relative path when specifying the dotfile's name in XML. Breathe expects the dotfile to be specified using a absolute path (and filename) as that was the default behavior in 1.9.2 or prior. TL;DR: don't specify the DOTFILE_DIRS tag in the doxyfile config! Currently, there is no plan to workaround using DOTFILE_DIRS tag.
Specifically for building breathe's docs. This is made possible by using the RST `ifconfig` directive. See #811
Per request, most of the namespaced member changes are reverted. Also reverted temporary changes to dot_graphs.cfg project as the fix was merged into master (which this branch should now be based on). Some changes seemed to have been lost since I did a `git reset --hard origin/branch-name` I have restored what I noticed, but there may be some other changes that were lost.
reverted another namespace change in domains.rst
mathjax output was currently broken due to changes in mathjax distribution path/URL. Updating to latest restores proper output.
Maybe the error about the `.. c:function::` signature is specific to my setup.
|
@michaeljones Sphinx still failed to recognize the C-specific array syntax. I can make it so the syntax looks like C++ (essentially ignoring the error) and open an issue describing the problem. That way we can merge this and circle back to that specific error (I suspect its a Sphinx problem). |
@2bndy5 Yeah I'm in favour of this approach, that way we can get this merged asap and handle this finicky edge case whenever it works out. Good idea! Edit: I'd like to do a final rebase prior to merging, so please @ me when it's ready. Thanks! |
|
I just did an interactive rebase on master. It should now be based on v4.34.0 release. I'll open an issue for the edge case, change the doc to ignore the edge case, and do another sweep through the diff (to make sure the rebase didn't have side effects). |
There was a problem hiding this comment.
@vermeeren I went through and verified all requests have been satisfied except for the c array syntax problem (which now has a devoted thread #854). I also combed through and made sure the rebase went well. ✅
For the record, I prefer these commits get squashed (its kinda my default on most projects). Lately, I've been a bit better at keeping a clean git history, and that's partly due to this experience (my contributions usually end up being learning opportunities).
|
Shall I squash and merge then? |
|
yeah, I think we're all onboard with that. |
|
Done. Thanks so much for all the hard work. Sorry it has dragged out for so long. Happy to see it merged. |
|
Happy to see this merged, again thanks a lot all!
@2bndy5 Good to hear this as well from my pov, cheers! |
Initially, this PR was meant to just fix all warnings/error in the docs builds, so we can catch bugs like #803 before release. In the process of resolving the warnings/errors, I had to resolve #811 by forcing both RTD and the docs CI workflow to use doxygen v1.9.3 (latest stable as of this writing).
I recommend inspecting the docs build artifacts from this PR's CI run(s).
Changes
upload the doxygen XML output as well as the built breathe docs as artifacts for the docs CI workflow
fix some errors present in the C++ test sources (at least those that weren't done on purpose). I also went through and suppressed any warnings emitted by doxygen for all the examples (tinyXML project is an absolute mess).
update mathjax_path in conf.py. It was broken since mathjax changed it distribution URL (I think for mathjax@v2)
fix some errors present in the rst sources (mostly the testing grounds).
Most of these errors related to improper use of namespaced members without documenting the necessary namespace (see bottom of domains.rst as example).
Others were simply duplicated IDs declared despite the use of
.. cpp:namespace::(probably not applicable to C only memebers). Thus, I had to inject some:no-link:options to a few directives' documentation.enforce code block highlighting on all literal code snippets
finally, change the
make htmlcommand to usesphinx-build -v -W -Ewhich will make the CI fail if the docs encounter any new warnings or errors from future contributions.I had to remove the
-n(sphinx's "nit-picky" mode) since all errors and warnings from that option are unavoidable (& usually ignored by RTD). With-nand-Woptions enabled, the docs' build will not/never pass.