Skip to content

Clean up doxygen comments in nestkernel directory#2113

Merged
jougs merged 54 commits intonest:masterfrom
jessica-mitchell:cleanup-cpp-docs
Jul 5, 2023
Merged

Clean up doxygen comments in nestkernel directory#2113
jougs merged 54 commits intonest:masterfrom
jessica-mitchell:cleanup-cpp-docs

Conversation

@jessica-mitchell
Copy link
Contributor

This PR aims to make the comments (doxygen and c++) more consistent and useful.
So far, only a few files have been touched because I would like some feedback as to whether I'm going in the right direction before proceeding. @jougs could you take a look?

Changes implemented:

  • C++ comments single line (max 2 lines if sentence is long): //
  • C++ comments multile line: /* * * * */
  • doxygen comments single line (max 2 lines if sentence is long): //!
  • doxygen comments multiline: /** * * * * */
  • doxygen comments side: //!<
  • avoid doxygen comments in .cpp files, move to header file
  • avoid duplicating code in comments; only include code(parameters, functions etc) that also have additional context, remove redundancies
  • include the variable name in functions in header file to match cpp file.

@jessica-mitchell jessica-mitchell added I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation. labels Jul 23, 2021
@jessica-mitchell jessica-mitchell marked this pull request as draft July 23, 2021 09:10
Copy link
Contributor

@jougs jougs 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 making the chaos and invonsistencies visible. I'm reviewing in greater detail once the crying stops. For now, I have a couple of generic comments:

  • I prefer the following doxygen style for functions and classes, even if the comment itself is only a single line:
    /**
     * ...
     */
    
    (I also think this is the predominant style presently used, so it will save you a lot of trouble down the line 😄)
  • I would use //! for long one-liners above variables and //!< for short ones behind them

Maybe we should set up a video session where we go through the Doxygen documentation and ask people what they (dis)like most?

@jessica-mitchell
Copy link
Contributor Author

Thanks for making the chaos and invonsistencies visible. I'm reviewing in greater detail once the crying stops. For now, I have a couple of generic comments:

* I prefer the following doxygen style for functions and classes, even if the comment itself is only a single line:
  ```
  /**
   * ...
   */
  ```

Sure, I think you're right that it's more common

* I would use `//!` for long one-liners above variables and `//!<` for short ones behind them

When you say above variables, can you give me an example of the syntax you'd expect this kind of comment? Where would not want this comment style? (it's just easier if I have an example to look up, because it's easy to get confused staring at slashes and askterisks all day ;) )

Maybe we should set up a video session where we go through the Doxygen documentation and ask people what they (dis)like most?

Yah sure, I think that could work.

@jougs
Copy link
Contributor

jougs commented Jul 26, 2021

Here are some examples that hopefully clarify my style preferences mentioned above:

/**
 * Set the minimal and maximal delay and override automatically determined values.
 */
void set_delay_extrema(long min_delay, long max_delay);

//! The variable min_delay holds the value for the smallest transmission delay in the network in steps
long min_delay_;

long max_delay_; //!< The largest transmission delay in the network (steps)

In real code, I would then actually prefer if similar variables were documented consistently, i.e. both min_delay_ and max_delay_ with either the long version, or with the short one, depending on the length of the comment. Above, I would likely prefer the short one, as the long description is a bit redundant and the short one would allow to skim through the code quicker (due to less optical clutter), cf.

//! The variable min_delay holds the value for the smallest transmission delay in the network in steps
long min_delay_;
//! The variable max_delay holds the value for the largest transmission delay in the network in steps
long max_delay_;
long min_delay_;   //!< The smallest transmission delay in the network (steps)
long max_delay_;   //!< The largest transmission delay in the network (steps)

@github-actions
Copy link

Pull request automatically marked stale!

@github-actions github-actions bot added the stale Automatic marker for inactivity, please have another look here label Oct 24, 2021
@jessica-mitchell jessica-mitchell removed the stale Automatic marker for inactivity, please have another look here label Dec 10, 2021
@github-actions
Copy link

github-actions bot commented May 2, 2023

Pull request automatically marked stale!

@github-actions github-actions bot added the stale Automatic marker for inactivity, please have another look here label May 2, 2023
@jessica-mitchell
Copy link
Contributor Author

@jougs the conflict is fixed

@terhorstd terhorstd requested a review from jougs June 27, 2023 11:41
@jessica-mitchell jessica-mitchell removed the stale Automatic marker for inactivity, please have another look here label Jul 4, 2023
@jessica-mitchell
Copy link
Contributor Author

@jougs ping!

@jougs
Copy link
Contributor

jougs commented Jul 4, 2023

@jessica-mitchell: pong!

Final run-through and further cleanup of differen types of comments
Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

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

Many thanks for this heroic effort and your patience. Merging without second review due to discussion in several meetings and because this "only" touches documentation.

@jougs jougs merged commit 55a005d into nest:master Jul 5, 2023
@jougs jougs changed the title Clean up comments in nestkernel Clean up doxygen comments in nestkernel directory Jul 5, 2023
@jessica-mitchell jessica-mitchell deleted the cleanup-cpp-docs branch September 13, 2024 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants

Comments