Skip to content

Add Doxygen build and fix all warnings#164

Merged
rapids-bot[bot] merged 24 commits intorapidsai:branch-0.36from
pentschev:doxygen
Jan 19, 2024
Merged

Add Doxygen build and fix all warnings#164
rapids-bot[bot] merged 24 commits intorapidsai:branch-0.36from
pentschev:doxygen

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.

@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.

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 <[email protected]>
Co-authored-by: Charles Blackmon-Luca <[email protected]>
@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

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