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

[doc] Add information regarding header include order #4020

Merged

Conversation

diivm
Copy link
Contributor

@diivm diivm commented May 3, 2020

closes #3904

@kunaltyagi kunaltyagi added this to the pcl-1.11.0 milestone May 3, 2020
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.

LGTM if it renders correctly as a webpage

doc/advanced/content/pcl_style_guide.rst Outdated Show resolved Hide resolved
@kunaltyagi kunaltyagi added changelog: enhancement Meta-information for changelog generation module: docs labels May 3, 2020
@kunaltyagi kunaltyagi changed the title [doc] Add header includes order [doc] Add information regarding header include order May 3, 2020
@kunaltyagi kunaltyagi added needs: author reply Specify why not closed/merged yet needs: code review Specify why not closed/merged yet labels May 3, 2020
@diivm
Copy link
Contributor Author

diivm commented May 3, 2020

Webpage looks fine 👍
Please squash while merging.

doc/advanced/content/pcl_style_guide.rst Outdated Show resolved Hide resolved
doc/advanced/content/pcl_style_guide.rst Outdated Show resolved Hide resolved
diivm and others added 2 commits May 3, 2020 17:26
Co-authored-by: SunBlack <SunBlack@users.noreply.github.com>
Co-authored-by: SunBlack <SunBlack@users.noreply.github.com>
Co-authored-by: SunBlack <SunBlack@users.noreply.github.com>
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.

Looks better to me

@kunaltyagi kunaltyagi removed the needs: author reply Specify why not closed/merged yet label May 3, 2020
@kunaltyagi
Copy link
Member

doc succeeded. Others cancelled manually

Copy link
Member

@SergioRAgostinho SergioRAgostinho left a comment

Choose a reason for hiding this comment

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

Minor actionable. Feel free to merge after the change.

Comment on lines 333 to 334
i. All modular PCL includes, except main includes of common module
#. The main PCL includes of common module
Copy link
Member

Choose a reason for hiding this comment

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

The concept of what is a "main include of common module" is not clear. I had to go to the clang-format file to understand what it represented. Provide examples for what a "normal modular include" and a "main include of a common module" are.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks good? 1f52a71

@kunaltyagi kunaltyagi added needs: more work Specify why not closed/merged yet and removed needs: code review Specify why not closed/merged yet labels May 7, 2020
@kunaltyagi kunaltyagi added needs: code review Specify why not closed/merged yet and removed needs: more work Specify why not closed/merged yet labels May 7, 2020
@diivm
Copy link
Contributor Author

diivm commented May 7, 2020

Do squash before merging ☺️

Co-authored-by: Sérgio Agostinho <sergio.r.agostinho@gmail.com>
@diivm diivm force-pushed the fix/header_includes branch from 57908d2 to 3a6fd33 Compare May 7, 2020 11:37
@SergioRAgostinho SergioRAgostinho merged commit cedac79 into PointCloudLibrary:master May 7, 2020
@diivm diivm deleted the fix/header_includes branch May 15, 2020 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: enhancement Meta-information for changelog generation module: docs needs: code review Specify why not closed/merged yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[docs] Ordering of header includes
4 participants