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 various documentation issues #2844

Open
wants to merge 37 commits into
base: next
Choose a base branch
from
Open

Fix various documentation issues #2844

wants to merge 37 commits into from

Conversation

ZedThree
Copy link
Member

This is a bid to try and get the number of Sphinx warnings down, to ideally start using error on warnings.

The major issue now is things like:

BOUT-dev/manual/sphinx/_breathe_autogen/file/pcr__thomas_8hxx.rst:4: WARNING: Duplicate C++ declaration, also defined at _breathe_autogen/file/pcr__thomas_8hxx:4.
Declaration is '.. cpp:function:: virtual void setCoefA (const Field2D &val)=0'.

which I think is complaining about overriding the pure virtual function. I don't know whether this is an issue with breathe or doxygen. Breathe is not super well maintained currently, so I might investigate other options.

I've fixed a massive chunk of these warnings by just not processing the .cxx files. I think this is probably reasonable, but it might involve going through and moving docstrings to the headers. That will take a little bit of time to do.

Making a draft now in case anyone has feedback or ideas on these changes.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

include/bout/sys/uuid.h Show resolved Hide resolved
@ZedThree
Copy link
Member Author

I've discovered that the vast majority of the warnings emitted from Sphinx are due to two separate bugs in doxygen and breathe.

Things that still need doing:

  • boutdata/boututils docs are still built as part of this repo, should probably build them separately
  • go through every single file and make sure the docstring is in the header

@ZedThree ZedThree marked this pull request as ready for review January 25, 2024 16:51
include/bout/generic_factory.hxx Show resolved Hide resolved
Comment on lines 34 to 35
sys.path.append("../../tools/pylib/boutpp")
sys.path.append("../../tools/pylib/boutconfig")
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

This was to stop generating the docs for the other directories under pylib, and to be explicit about what's built here. It looks like it built the docs for everything except these two, which is very odd. Not sure what's going on!

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the other ones get pip installed?
I haven't checked though!

Anyway, for zoidberg we currently don't build the docs but reference the bout++ docs, so if you want to drop zoidberg here, we should add zoidberg independently.
Also, it would be better to remove the sections, rather then to have them being empty because python cannot find the modules.

dschwoerer and others added 4 commits March 1, 2024 20:43
include/bout/generic_factory.hxx is licensed under the MIT License
based on Modern C++ Design/Loki by A. Alexandrescu
Update + add License for #2844 Fix various documentation issues
This reverts commit e6bc5c5.

The commit tried to make boutuitls and boutdata non-importable for the
documentation tool.  If those modules should be remove from the
documentation, this can be achieved by removing them from
manual/sphinx/_apidoc/. This should not break the documentation fro
boutpp or boutconfig.
bendudson and others added 6 commits March 21, 2024 09:07
Revert "Docs: Don't include boutdata/boututils in docs"
* next: (481 commits)
  CI: Show more output of dnf5 to help debugging
  CI: Disable annotations for clang-tidy-review
  CI: Bump clang-tidy-review
  Remove coverage test from github workflow
  CMake: Ensure we find ADIOS2 if we downloaded it
  Use modern `output` API
  Remove unused header include
  Remove unused `x_outer_dirichlet` member from LaplaceXY2 hypre
  Change test hypre type to `gmres`
  Fix laplacexy2-hypre test failing silently
  CMake: Bump downloaded SUNDIALS version
  CMake: Reduce duplication in linking against adios2
  CMake: Ensure we find adios2 when linking against BOUT++
  Apply clang-format changes
  hypre_interface: Add print() method to HypreMatrix
  tests/integrated: Fix hypre tests
  OptionsNetCDF: verifyTimesteps, write override base class
  Testing: Missed 3D + hypre combination
  Disable hypre with 3D metrics
  Prefer scientific notation
  ...
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@@ -261,7 +262,7 @@ class sha1 {
size_t m_byteCount;
};

static std::mt19937 clock_gen(std::random_device{}());
static std::mt19937 clock_gen{std::random_device{}()};
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: variable 'clock_gen' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]

static std::mt19937 clock_gen{std::random_device{}()};
                    ^

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