meshula
commented
1 month ago Overall this is a great improvement.
I'm not familiar with ABBREVIATE_BRIEF. Does it preclude documenting the parameters formally?
Closed darbyjohnston closed 2 weeks ago
meshula
commented
1 month ago Overall this is a great improvement.
I'm not familiar with ABBREVIATE_BRIEF. Does it preclude documenting the parameters formally?
darbyjohnston
commented
1 month ago The ABBREVIATE_BRIEF option is a bit odd, it filters the text to try and make it more concise. So if your comment looks like this:
/// The Addition class provides functionality for adding numbers.
It will be changed to: "Functionality for adding numbers". I always find it a bit surprising so I like to turn it off.
If these changes look OK how should we go about documenting the rest of the code? It would be nice to do the work in chunks to keep it manageable, maybe one PR per class?
meshula
commented
1 month ago My first question about ABBREVIATE_BRIEF was really poking at whether that was supposed to do more, like infer parameters and returns.
I'd prefer we go heavy-handed with documentation, ie with full adornment. I assume ABBREVIATE_BRIEF means that we can skip the @brief tag. Sometimes this feels overboard, but I think it's better to be thorough and consistent in order that all the tooling around doxygen syntax can come into play. e.g. I just did a similar pass on Nanocolor, where everything is this heavy. Doing it this way allowed me to automate the decoration of the entire library using various tools in VsCode, so it wasn't as terrible as it seems. (Right click function, "Add doxygen comment", more or less)
/**
* @brief Converts a XYZ color to RGB color space.
*
* Converts a XYZ color to RGB color space using the provided color space.
*
* @param cs Pointer to the color space object.
* @param xyz The XYZ color to convert.
* @return The RGB color.
*/
NCAPI NcRGB NcXYZToRGB(const NcColorSpace* cs, NcXYZ xyz);The ABBREVIATE_BRIEF option doesn't do any inference, it's just for filtering text. To automate brief descriptions I was trying the options MULTILINE_CPP_IS_BRIEF=YES and REPEAT_BRIEF=YES, so a comment like this:
/// Converts a XYZ color to RGB color space. The conversion is performed
/// using the provided color space.Will have the first sentence used for the brief description, and the entire text used for the full description.
I like the @param and @return keywords, though I wonder if we can omit them for simple functions, especially class member get methods?
meshula
commented
1 month ago Sure, why don't we have a look at that, it sounds fine to me.
darbyjohnston
commented
1 month ago After more testing the MULTILINE_CPP_IS_BRIEF option didn't work very well so I turned it off and added explicit @brief markup as you suggested.
I think the detailed description, @param markup, and @return markup should be optional since it adds a lot of boiler plate for simple functions like member get functions. They should just be used where necessary to provide extra information that isn't obvious from the function name.
Some other notes:
/// for comments since it's a bit easier to type than the C style comments, but can change that if there is already a precedent for using the C style comments.darbyjohnston
commented
1 month ago I created a new issue since the original Doxygen issue was closed awhile back. Given the amount of work maybe we can use the issue as an umbrella for multiple PRs: #1753
Any ideas on how to fix the CI build, the error doesn't seem related to this change?
meshula
commented
1 month ago The problem in the CI is that pip on windows under msys is failing because distlib can't be found. @reinecke does that ring any bells? I feel like we struggled through something like that last year but the details escape me.
darbyjohnston
commented
1 month ago Strange, it looks like distlib is being installed as part of the msys setup:
installing mingw-w64-x86_64-python-distlib...
codecov-commenter
commented
2 weeks ago All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 81.71%. Comparing base (
c0e97b0) to head (2723cf9). Report is 11 commits behind head on main.
darbyjohnston
commented
2 weeks ago Sorry, was trying to follow the GitHub rebasing steps and seemed to have created a mess again... I may just create a new PR instead of trying to fix this one: #1765
Link the Issue(s) this Pull Request is related to.
Fixes #1753
Summarize your change.
Adds Doxygen comments to the RationalTime class. These comments are based on the Python doc strings with some editing for consistency. A couple of notes:
Here is a screenshot of the Doxygen comments providing help in Visual Studio:
Here is a screenshot of the HTML generated for RationalTime: