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.

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

[WIP] Squash doxygen errors. #357

Merged
merged 13 commits into from
Mar 5, 2018
Merged

[WIP] Squash doxygen errors. #357

merged 13 commits into from
Mar 5, 2018

Conversation

sniyaz
Copy link

@sniyaz sniyaz commented Mar 2, 2018

See #329.

This mainly involved fixing some doc-strings and renaming parameters.

TODO: I had some trouble squashing the warnings involving SplineTrajectory-impl.hpp. @jslee02 and other people who know doxygen better, could take a look? There doesn't appear to be an documentation in that file at all, so I'm confused 😖


Before creating a pull request

  • Document new methods and classes
  • Format code with make format

Before merging a pull request

  • Set version target by selecting a milestone on the right side
  • Summarize this change in CHANGELOG.md
  • Add unit test(s) for this change

@sniyaz sniyaz requested review from brianhou and jslee02 March 2, 2018 22:39
@codecov
Copy link

codecov bot commented Mar 2, 2018

Codecov Report

Merging #357 into master will increase coverage by 0.01%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master     #357      +/-   ##
==========================================
+ Coverage   80.98%   80.99%   +0.01%     
==========================================
  Files         206      206              
  Lines        6036     6036              
==========================================
+ Hits         4888     4889       +1     
+ Misses       1148     1147       -1
Impacted Files Coverage Δ
include/aikido/statespace/GeodesicInterpolator.hpp 100% <ø> (ø) ⬆️
...nclude/aikido/planner/ompl/GeometricStateSpace.hpp 100% <ø> (ø) ⬆️
include/aikido/statespace/SO3.hpp 100% <ø> (ø) ⬆️
include/aikido/statespace/CartesianProduct.hpp 100% <ø> (ø) ⬆️
include/aikido/constraint/Satisfied.hpp 100% <ø> (ø) ⬆️
include/aikido/statespace/dart/RnJoint.hpp 100% <ø> (ø) ⬆️
include/aikido/statespace/detail/SO3-impl.hpp 100% <ø> (ø) ⬆️
include/aikido/common/Spline.hpp 100% <ø> (ø) ⬆️
include/aikido/statespace/StateSpace.hpp 100% <ø> (ø) ⬆️
include/aikido/constraint/dart/CollisionFree.hpp 100% <ø> (ø) ⬆️
... and 22 more

@sniyaz
Copy link
Author

sniyaz commented Mar 3, 2018

Hang on, flipped something. Will fix to satisfy Travis.

Copy link
Member

@jslee02 jslee02 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! Left a few suggestions.

/// \param group Collision group.
void addSelfCheck(std::shared_ptr<::dart::collision::CollisionGroup> _group);
/// \param _group Collision group.
void addSelfCheck(std::shared_ptr<dart::collision::CollisionGroup> _group);
Copy link
Member

@jslee02 jslee02 Mar 3, 2018

Choose a reason for hiding this comment

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

Nit: Was this one of the reasons of the Doxygen warnings? We used global namespace resolutions for dart (i.e., ::dart) to avoid conflicts with ::aikido::constraint::dart. It might be good to leave it as it was if it's not a cause of the warnings.

Probably this is the reason for the CI fails.

void removeSelfCheck(
std::shared_ptr<::dart::collision::CollisionGroup> _group);
/// \param _group Collision group.
void removeSelfCheck(std::shared_ptr<dart::collision::CollisionGroup> _group);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Was this one of the reasons of the Doxygen warnings? We used global namespace resolutions for dart (i.e., ::dart) to avoid conflicts with ::aikido::constraint::dart. It might be good to leave it as it was if it's not a cause of the warnings.

@@ -31,7 +31,6 @@ class SO3StateHandle : public statespace::StateHandle<SO3, _QualifiedState>

/// Constructs a point in SO(3) from a unit quaternion.
///
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Could you remove this empty line as well?

@sniyaz sniyaz added this to the Aikido 0.3.0 milestone Mar 5, 2018
@sniyaz
Copy link
Author

sniyaz commented Mar 5, 2018

I've squashed some errors from the Robot API and deleted SplineTrajectory-impl.hpp. Should be good to go now. Please rubber stamp when you have the chance 😄

Copy link
Member

@jslee02 jslee02 left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for this work!

@@ -82,8 +82,10 @@ class BarrettFingerKinematicSimulationPositionCommandExecutor
/// is reached, or collision is detected.
void step(const std::chrono::system_clock::time_point& timepoint) override;

/// \copydoc
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's figure out how to fix this so we don't need to duplicate the \copydocs.

///
/// \param collideWith CollisionGroup to check finger collisions
/// \return false if collideWith cannot be set (during execution)
/* clang-format off */
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Could we use // clang-format off (C++ style) instead of /* clang-format off */ (C style)?

@sniyaz
Copy link
Author

sniyaz commented Mar 5, 2018

@brianhou I tried to look up ways to turn break the copydoc into multiple lines, but the only solution seems to manually turning clang format on/off 😢

@brianhou brianhou merged commit 7d83f5a into master Mar 5, 2018
@brianhou brianhou deleted the sniyaz/doxygen branch March 5, 2018 23:42
@brianhou brianhou mentioned this pull request Mar 5, 2018
@brianhou brianhou mentioned this pull request May 9, 2018
5 tasks
gilwoolee pushed a commit that referenced this pull request Jan 21, 2019
* Move constraint to postprocess call, need to fix tests still.

* Squished about half of warnings.

* Squash most errors.

* Fix test_SmoothPostProcessor.

* Fix build, respond to nits.

* Remove wierd -impl.hpp file that was never used.

* Clean up doxygen errors from Robot API.

* Update CHANGELOG.

* Update CHANGELOG.md

* Put copydoc back in, turn off clang format.

* Change clang format comments to C++ style.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants