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

Publish C++ developer docs #11475

Merged
merged 13 commits into from
Sep 12, 2022

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Aug 4, 2022

Description

This PR adds the developer documentation to our doxygen. The changes to enable this are minimal: the files have been moved from cpp/docs to cpp/doxygen and added to the INPUT section of the Doxyfile, and a custom DoxygenLayout.xml has been added to include the necessary header (note that this file was autogenerated with doxygen -l and the only modification was adding one tab for the developer guide). I added an anchor to the main developer guide header so that it can be linked to from the header.

Our current developer guide was written to be viewed on Github. As a result, it uses some Github Flavored Markdown extensions, primarily around the way that code is displayed. Since doxygen does not support those, I had to make some modifications to the contents of the docs so that they would render the same way. Some care is needed around escaping the @ symbol in the right locations in the docs to prevent doxygen from interpreting commands in examples. Finally due to doxygen/doxygen#6054 we cannot put certain commands inside code blocks and get them rendered correctly. However, with a few cycles of rebuilding and checking the output I was able to get everything to look correct. These changes mean that the guide will no longer look as nice as before when viewed directly on Github. However, the goal of this PR is to allow everyone to move towards viewing the built and published documentation instead, so this isn't a problem.

This is how it looks now:

image

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@vyasr vyasr added doc Documentation libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Aug 4, 2022
@vyasr vyasr self-assigned this Aug 4, 2022
@vyasr vyasr removed the improvement Improvement / enhancement to an existing function label Aug 4, 2022
@vyasr vyasr marked this pull request as ready for review August 29, 2022 22:03
@vyasr vyasr requested a review from a team as a code owner August 29, 2022 22:03
@vyasr vyasr requested review from harrism and PointKernel August 29, 2022 22:04
@codecov
Copy link

codecov bot commented Aug 29, 2022

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.10@ecf4662). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@               Coverage Diff               @@
##             branch-22.10   #11475   +/-   ##
===============================================
  Coverage                ?   86.42%           
===============================================
  Files                   ?      145           
  Lines                   ?    23009           
  Branches                ?        0           
===============================================
  Hits                    ?    19885           
  Misses                  ?     3124           
  Partials                ?        0           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

cpp/doxygen/developer_guide/DOCUMENTATION.md Outdated Show resolved Hide resolved
cpp/doxygen/developer_guide/DOCUMENTATION.md Outdated Show resolved Hide resolved
cpp/doxygen/developer_guide/DOCUMENTATION.md Outdated Show resolved Hide resolved
cpp/doxygen/developer_guide/TESTING.md Show resolved Hide resolved
@@ -91,7 +91,7 @@ TYPED_TEST(TypedTestFixture, FirstTest){
}
```

To specify the list of types to use, instead of GTest's `::testing::Types<...>`, libcudf provides `cudf::test::Types<...>` which is a custom, drop-in replacement for `::testing::Types`.
To specify the list of types to use, instead of GTest's `testing::Types<...>`, libcudf provides `cudf::test::Types<...>` which is a custom, drop-in replacement for `testing::Types`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little confused about why the changes to the ::testing namespace were needed. The code appears to mix styles between ::testing and testing.

Copy link
Member

Choose a reason for hiding this comment

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

It's normal in gtest code to use ::testing::... This is the style used in the gtest documentation. I'm not sure why, though...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My guess is it's a matter of being explicit and avoiding namespace clashes since it's probably quite common for other projects making use of gtest to have their own testing namespaces. The leading :: guarantees global scope resolution.

As for why I had to change it, I am not entirely sure what doxygen's scope resolution rules are but in some cases (like this one) the leading :: leads to doxygen failing to find the right link. I get errors like this from the pre-commit doxygen check:

cudf/cpp/doxygen/developer_guide/TESTING.md:67: warning: explicit link request to 'testing::Test' could not be resolved

I chose to remove the leading colons for expediency to get past this issue since I really have no idea how to resolve it more generally. My best guess is that the global namespace in the context of this document is some doxygen namespace that doesn't contain anything, so the name lookup fails. I don't know how else to resolve this, and I think it's a minor enough issue that it's OK to move forward anyway. If you can find a better solution I'm all for it, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't want to hold up the PR on this but let's leave this conversation unresolved.

cpp/doxygen/developer_guide/TESTING.md Outdated Show resolved Hide resolved
cpp/doxygen/modify_fences.sh Outdated Show resolved Hide resolved
cpp/doxygen/modify_fences.sh Outdated Show resolved Hide resolved
cpp/doxygen/modify_fences.sh Outdated Show resolved Hide resolved
@vyasr vyasr requested a review from bdice September 7, 2022 23:25
cpp/doxygen/Doxyfile Outdated Show resolved Hide resolved
@vyasr
Copy link
Contributor Author

vyasr commented Sep 12, 2022

@gpucibot merge

@vyasr
Copy link
Contributor Author

vyasr commented Sep 12, 2022

rerun tests

@rapids-bot rapids-bot bot merged commit d6952ba into rapidsai:branch-22.10 Sep 12, 2022
@vyasr vyasr deleted the feature/publish_cpp_dev_docs branch September 12, 2022 22:59
rapids-bot bot pushed a commit to rapidsai/cuspatial that referenced this pull request Sep 30, 2022
This PR mimics rapidsai/cudf#11475 to add libcuspatial's C++ developer guide to the rendered Doxygen HTML docs. See that PR for detailed discussion.

Contributes to #598 . Closes #235

Note that this PR depends on #672 and #667 so the changed files will include changes from those PRs until they are merged.

Authors:
  - Mark Harris (https://github.com/harrism)

Approvers:
  - H. Thomson Comer (https://github.com/thomcom)
  - Vyas Ramasubramani (https://github.com/vyasr)
  - AJ Schmidt (https://github.com/ajschmidt8)
  - Michael Wang (https://github.com/isVoid)

URL: #673
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Documentation libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants