Skip to content

Improved docstring consistency across the codebase #2087

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

im23123v
Copy link

@im23123v im23123v commented Mar 2, 2025

Hello Developers,

This is my 1st contribution to open source, and I'm excited to contribute to this project!

This PR Fixes : #2064
Changes Made:

  • Standardized the docstring format across the codebase.
  • Ensured uniform usage of @brief in comments.
  • Improved formatting for better readability and consistency.
  • Verified that all modified files now follow the same documentation style.

I have carefully reviewed my changes, but I would love your feedback!
Looking forward to your review and suggestions.
@christophfroehlich @saikishor

Regards,
Vishwanath Karne

Copy link
Contributor

@christophfroehlich christophfroehlich 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 housekeeping. Please always run pre-commit or install it to do the checks automatically.

This will take some time to review though..

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this.

/**
* \return name.
*/
/// Gets the name of the actuator hardware.
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need the lines starting with /// across all files?

Copy link
Author

Choose a reason for hiding this comment

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

I followed the existing style in most files but can update it if needed. Would you like me to revert /// to /** ... */ in some places?

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 think we need this information within /// and then in the @brief again?

Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

I believe it is not compiling for this. Please fix it and make sure the project compiles. Also, please install pre-commit as @christophfroehlich already mentioned.

@christophfroehlich
Copy link
Contributor

I can imagine that this was a lot of work. But with respect to the huge amount of changes, could you please split this PR into smaller portions? For example, first fixing the format/syntax of the docstring in one PR without changing/rephrasing them?

Co-authored-by: Sai Kishor Kothakota <saisastra3@gmail.com>
* the controller can be activated and to claim interfaces from the hardware.
*
* The claimed interfaces are populated in the
* `ControllerInterfaceBase::state_interfaces_` member.
Copy link
Member

Choose a reason for hiding this comment

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

Things like * \ref ControllerInterfaceBase::state_interfaces_ "state_interfaces_" member. create hyperlink to that class in doxygen. Is that not the case anymore?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rework docstrings to have consistent style
4 participants