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.

Sign up for GitHub

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

Jump to bottom

Add missing documentation in scalar/ headers #10861

Merged
merged 6 commits into from
May 18, 2022
Merged

Add missing documentation in scalar/ headers #10861

merged 6 commits into from
May 18, 2022

Conversation

karthikeyann
Copy link
Contributor

@karthikeyann karthikeyann commented May 16, 2022 •
edited
Loading

fixes part of #9373

Add missing documentation in scalar/ headers to fix doxygen warnings

@karthikeyann
add missing doc in scalar.hpp, fixes some warnings
73315aa
@karthikeyann
proposal @briefreturn, @movedoc
c8d5598 
@briefreturn = @brief + @return for same description and return.
@movedoc = move constructor doc. [ideally doxygen should skip this, not
supported now]
@karthikeyann karthikeyann added 3 - Ready for Review Ready for review by team doc Documentation 4 - Needs Review Waiting for reviewer to review or respond non-breaking Non-breaking change labels May 16, 2022
@karthikeyann karthikeyann self-assigned this May 16, 2022
@karthikeyann karthikeyann requested a review from a team as a code owner May 16, 2022 11:11
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label May 16, 2022
@karthikeyann karthikeyann requested a review from bdice May 16, 2022 11:23
@codecov
Copy link

codecov bot commented May 16, 2022 •
edited
Loading

Codecov Report

Merging #10861 (20a8995) into branch-22.06 (e58d049) will increase coverage by 0.03%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##           branch-22.06   #10861      +/-   ##
================================================
+ Coverage         86.29%   86.33%   +0.03%     
================================================
  Files               144      144              
  Lines             22656    22665       +9     
================================================
+ Hits              19552    19567      +15     
+ Misses             3104     3098       -6
Impacted Files Coverage Δ
python/dask_cudf/dask_cudf/tests/test_groupby.py 100.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/groupby.py 97.42% <0.00%> (+<0.01%) ⬆️
python/cudf/cudf/core/dataframe.py 93.78% <0.00%> (+0.04%) ⬆️
python/cudf/cudf/core/column/string.py 88.78% <0.00%> (+0.12%) ⬆️
python/cudf/cudf/core/groupby/groupby.py 91.79% <0.00%> (+0.22%) ⬆️
python/cudf/cudf/core/tools/datetimes.py 84.49% <0.00%> (+0.30%) ⬆️
python/cudf/cudf/core/column/lists.py 91.70% <0.00%> (+0.97%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e58d049...20a8995. Read the comment docs.

bdice
bdice reviewed May 16, 2022
View reviewed changes
cpp/include/cudf/scalar/scalar.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/scalar/scalar.hpp Outdated Show resolved Hide resolved
@karthikeyann karthikeyann requested a review from bdice May 17, 2022 06:04
@karthikeyann karthikeyann changed the title Add doxygen aliases for documentation Add missing documentation in scalar/ headers May 17, 2022
bdice
bdice approved these changes May 17, 2022
View reviewed changes
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

A handful of small suggestions, which I'll let you resolve as you wish. None of them are blocking, so I'll approve this and let you fix (or not fix) whichever ones you wish. Overall I think the explicit definitions are better than using aliases. The duplication is not too painful, and the docs are easy to understand and write without the non-standard syntax.

cpp/include/cudf/scalar/scalar.hpp Show resolved Hide resolved
cpp/include/cudf/scalar/scalar_factories.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/scalar/scalar.hpp Outdated Show resolved Hide resolved
vyasr
vyasr approved these changes May 17, 2022
View reviewed changes
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes! +1 for avoiding aliases, in these situations I think in the long run they end up causing more confusion than helping.

@karthikeyann
Copy link
Contributor Author

karthikeyann commented May 18, 2022 •
edited
Loading

Thank you @bdice and @vyasr

@karthikeyann
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 7b393a7 into rapidsai:branch-22.06 May 18, 2022
@karthikeyann karthikeyann removed 3 - Ready for Review Ready for review by team 4 - Needs Review Waiting for reviewer to review or respond labels May 20, 2022
@karthikeyann karthikeyann mentioned this pull request May 21, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bdice bdice bdice approved these changes

@vyasr vyasr vyasr approved these changes

@robertmaynard robertmaynard Awaiting requested review from robertmaynard robertmaynard is a code owner automatically assigned from rapidsai/cudf-cpp-codeowners

Assignees

@karthikeyann karthikeyann

Labels
doc Documentation libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@karthikeyann @vyasr @bdice