-
Notifications
You must be signed in to change notification settings - Fork 28
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
Conversation
@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 |
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.
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.
This is a great idea, I didn't know |
@@ -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" |
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.
Reasoning:
- cancelation: codespell corrects it to the British English spelling "cancellation" (with two
l
s), whereas we chose the American spelling "cancelation(with one
l`) 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".
Co-authored-by: Charles Blackmon-Luca <20627856+charlesbluca@users.noreply.github.com>
Thanks again @charlesbluca , I've addressed the points you raised. 🙂 |
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.
Thanks @pentschev!
Thanks for the review @charlesbluca ! |
/merge |
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
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.