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

Update documentation to be coherent with the style guide #4771

Merged

Conversation

tin1254
Copy link
Contributor

@tin1254 tin1254 commented May 24, 2021

  • update include guard to #pragma once
  • fix broken link to the style guide

The include guard bilateral.hpp is still in the old style, I will update it when I refactor other filters later

Update include guard to #pragma once, fix link to style guide
kunaltyagi
kunaltyagi previously approved these changes May 24, 2021
Copy link
Member

@kunaltyagi kunaltyagi left a comment

Choose a reason for hiding this comment

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

Might have to see visually to verify, but that's after merge

@kunaltyagi kunaltyagi added changelog: fix Meta-information for changelog generation module: docs labels May 24, 2021
@@ -1175,8 +1154,7 @@ And the *bilateral.hpp* likes:
*
*/

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could use this PR to update also the LICENSE blurb to use the new one when writing a class. What do you think @kunaltyagi ?

Copy link
Member

Choose a reason for hiding this comment

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

Blurb in this rst file? Sure. But not in actual code files

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, speaking about the doc, not actual code!

Copy link
Contributor Author

@tin1254 tin1254 May 24, 2021

Choose a reason for hiding this comment

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

This one? Maybe we should also add this to the style guide in a new PR

Copy link
Member

Choose a reason for hiding this comment

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

Would it be considered a style guide? Isn't that more for code? We do have a blurb in contributing.md for reference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally I would consider it as style guide (Google Style Guide also include in their guide)

But maybe updating this file is enough for new contributors, as anyone wanted to implement a new class will definitely look at this rst

@tin1254
Copy link
Contributor Author

tin1254 commented May 24, 2021

also I found the links to the class are also broken (e.g. `:pcl:pcl::Filter<pcl::Filter>` in the page), we may want to update the doxylink here from http://docs.pointclouds.org/trunk/ to https://pointclouds.org/documentation/

but I'm not sure if it's correct as I didn't use doxylink before

@kunaltyagi
Copy link
Member

Paging @larshg for assist in doxylink

Copy link
Member

@kunaltyagi kunaltyagi left a comment

Choose a reason for hiding this comment

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

This changeset looks self-contained. Please use other PRs for doxylink corrections or other improvements :)

Copy link
Member

@mvieth mvieth left a comment

Choose a reason for hiding this comment

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

Nice!

@mvieth mvieth merged commit 4e7c566 into PointCloudLibrary:master May 25, 2021
@larshg
Copy link
Contributor

larshg commented May 25, 2021

Paging @larshg for assist in doxylink

I'm not familiar with doxylink or the like, sorry.

@tin1254 tin1254 deleted the update_write_new_class_tutorial branch May 25, 2021 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: fix Meta-information for changelog generation module: docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants