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

Add Doxygen build and fix all warnings #164

Merged
merged 24 commits into from
Jan 19, 2024

Conversation

pentschev
Copy link
Member

The C++ code was documented for some time but Doxygen build process was not included. This change now introduces Doxygen builds and fixes all documentation warnings.

@pentschev pentschev requested review from a team as code owners January 12, 2024 23:21
@pentschev
Copy link
Member Author

@charlesbluca @wence- I'd appreciate your review here sometime next week. Sorry that it is such a long PR, fortunately ~2.8k lines are just the Doxyfile, whereas the remaining is mostly missing documentation.

Copy link
Member

@charlesbluca charlesbluca left a comment

Choose a reason for hiding this comment

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

Thanks @pentschev! So far I've only looked into the files generating the documentation, will follow up on the added docstrings themselves - would it make sense to add codespell as a pre-commit check in this PR as well? There are a few typos that might be easier to review and correct by adding that check and running through the changed files.

ci/checks/doxygen.sh Outdated Show resolved Hide resolved
ci/checks/doxygen.sh Outdated Show resolved Hide resolved
@pentschev
Copy link
Member Author

would it make sense to add codespell as a pre-commit check in this PR as well? There are a few typos that might be easier to review and correct by adding that check and running through the changed files.

This is a great idea, I didn't know codespell but was wondering if something like that existed. I've now added codespell and fixed typos it identified.

@@ -100,3 +100,6 @@ cmake.minimum-version = "3.26.4"
ninja.make-fallback = true
sdist.reproducible = true
wheel.packages = ["ucxx"]

[tool.codespell]
ignore-words-list = "cancelation,inflight"
Copy link
Member Author

Choose a reason for hiding this comment

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

Reasoning:

  • cancelation: codespell corrects it to the British English spelling "cancellation" (with two ls), whereas we chose the American spelling "cancelation(with onel`) as it is the one used by UCX;
  • inflight: that's how we've been writing it and there are many instances of the word "inflight" online, although dictionaries seem to point "in-flight" as correct codespell didn't find all its instances automatically so for now I ignored it, but we can review it if the consensus is we should definitely switch to "in-flight".

pentschev and others added 2 commits January 17, 2024 20:32
Co-authored-by: Charles Blackmon-Luca <20627856+charlesbluca@users.noreply.github.com>
Co-authored-by: Charles Blackmon-Luca <20627856+charlesbluca@users.noreply.github.com>
@pentschev
Copy link
Member Author

Thanks again @charlesbluca , I've addressed the points you raised. 🙂

Copy link
Member

@charlesbluca charlesbluca left a comment

Choose a reason for hiding this comment

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

Thanks @pentschev!

@pentschev
Copy link
Member Author

Thanks for the review @charlesbluca !

@pentschev
Copy link
Member Author

/merge

@rapids-bot rapids-bot bot merged commit 2249bed into rapidsai:branch-0.36 Jan 19, 2024
21 checks passed
jameslamb pushed a commit to jameslamb/ucxx that referenced this pull request Jan 22, 2024
The C++ code was documented for some time but Doxygen build process was not included. This change now introduces Doxygen builds and fixes all documentation warnings.

Authors:
  - Peter Andreas Entschev (https://github.com/pentschev)

Approvers:
  - Jake Awe (https://github.com/AyodeAwe)
  - Charles Blackmon-Luca (https://github.com/charlesbluca)

URL: rapidsai#164
@pentschev pentschev deleted the doxygen branch February 7, 2024 13:23
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