-
Notifications
You must be signed in to change notification settings - Fork 273
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 comments to RationalTime #1746
base: main
Are you sure you want to change the base?
Add Doxygen comments to RationalTime #1746
Conversation
Signed-off-by: Darby Johnston <darbyjohnston@yahoo.com>
Overall this is a great improvement. I'm not familiar with ABBREVIATE_BRIEF. Does it preclude documenting the parameters formally? |
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:
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? |
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
|
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:
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? |
Sure, why don't we have a look at that, it sounds fine to me. |
Signed-off-by: Darby Johnston <darbyjohnston@yahoo.com>
After more testing the MULTILINE_CPP_IS_BRIEF option didn't work very well so I turned it off and added explicit I think the detailed description, Some other notes:
|
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.
For more complex functions we could use @return, but I don't spot anything here that really warrant's it. I think this is a good pattern to start with.
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? |
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. |
Strange, it looks like distlib is being installed as part of the msys setup:
|
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: