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

docs(contributing): update the coding guideline #599

Merged

Conversation

Shin-kyoto
Copy link
Contributor

@Shin-kyoto Shin-kyoto commented Aug 19, 2024

Description

Updated the coding guideline.

To the reviewers:

  • Please check whether there are any inappropriate guidelines included.
  • In particular, verify if my thoughts on new/delete/free/raw pointers and throw are correct.
  • If there are any other points that should be mentioned, please let me know. I'd like to add anything that can be easily incorporated.
  • Please confirm if the structure of the text, the subtitles, and the paragraph divisions are appropriate.
  • Regarding the ROS nodes guidelines in the ROS 2 Style Guide, I'm not sure where the link should point to, so I've linked it to class-design.md. If there is a better option, please let me know.
  • Since I think that the coding guideline document is still incomplete, I have left the "Under Construction" note. Please let me know if it should be removed.

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The Reviewers must check the checkboxes below before approval.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

Signed-off-by: Shin-kyoto <58775300+Shin-kyoto@users.noreply.github.com>
@Shin-kyoto Shin-kyoto added type:documentation Creating or refining documentation. tag:deploy-docs Mark for deploy-docs action generation. (used-by-ci) labels Aug 19, 2024
@Shin-kyoto
Copy link
Contributor Author

Shin-kyoto commented Aug 19, 2024

@xmfcx @YossyTaka @ytakano
Please add the comments if you have any idea about the coding guideline 🙏

Signed-off-by: Shin-kyoto <58775300+Shin-kyoto@users.noreply.github.com>
Signed-off-by: Shin-kyoto <58775300+Shin-kyoto@users.noreply.github.com>
Shin-kyoto and others added 6 commits August 19, 2024 22:11
Signed-off-by: Shin-kyoto <58775300+Shin-kyoto@users.noreply.github.com>
…easily

Co-authored-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
…iate

Co-authored-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
Signed-off-by: Shin-kyoto <58775300+Shin-kyoto@users.noreply.github.com>
…header files to be exported

Signed-off-by: Shin-kyoto <58775300+Shin-kyoto@users.noreply.github.com>
…iate

Signed-off-by: Shin-kyoto <58775300+Shin-kyoto@users.noreply.github.com>
@Shin-kyoto Shin-kyoto requested a review from youtalk August 19, 2024 14:05
Copy link
Member

@youtalk youtalk left a comment

Choose a reason for hiding this comment

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

Thank you for your updates.

Copy link

@sasakisasaki sasakisasaki left a comment

Choose a reason for hiding this comment

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

I have read all contents in the index.md and understood this is informative notes. Thank you so much!

Signed-off-by: Shin-kyoto <58775300+Shin-kyoto@users.noreply.github.com>
Signed-off-by: Shin-kyoto <58775300+Shin-kyoto@users.noreply.github.com>
@Shin-kyoto Shin-kyoto requested a review from mitsudome-r August 20, 2024 02:48
@mitsudome-r
Copy link
Member

The guideline itself generally looks okay, but most of them seem to be duplicate of the materials in other pages like:

Could you explain the purpose of this change. Is your intention to add a summary page that require particular attention?

In that case, I'm okay to add Autoware specific guidelines mentioned in this summary page(like adding autoware_ prefix to package names), but I rather keep the details about C++ guidelines as a separate page and just put a link to that page. It would make us harder to keep consistency within our documentation if there are duplicate materials explained in different places.

@Shin-kyoto
Copy link
Contributor Author

Shin-kyoto commented Aug 20, 2024

The guideline itself generally looks okay, but most of them seem to be duplicate of the materials in other pages like:

Could you explain the purpose of this change. Is your intention to add a summary page that require particular attention?

In that case, I'm okay to add Autoware specific guidelines mentioned in this summary page(like adding autoware_ prefix to package names), but I rather keep the details about C++ guidelines as a separate page and just put a link to that page. It would make us harder to keep consistency within our documentation if there are duplicate materials explained in different places.

Thank you for your good comment!!
I’m considering making changes based on the following approach. What do you think about it? @mitsudome-r

  • Move the content related to C++ and ROS to the separated pages.
  • Document Autoware-specific guidelines on this page.

@TakaHoribe
Copy link
Contributor

@Shin-kyoto Thank you for your work. Establishing coding guidelines is a great activity as it clarifies the criteria for PRs and reduces review time.

As @mitsudome-r mentioned, the document contains information with different levels of detail and some overlap. For example, the using .at(i) instead of [i], and the using the autoware_ prefix in packages are very different in scope.

It would be helpful to explain why each item is included on this page clearly. For instance, essential points like using the autoware_ prefix should be clearly stated. On the other hand, for more specific details (like index access or logger usage), it might be better to phrase it as, “Here are some frequent issues raised in reviews or static analysis, referenced from the C++/ROS guidelines".

@Shin-kyoto
Copy link
Contributor Author

As @mitsudome-r mentioned, the document contains information with different levels of detail and some overlap. For example, the using .at(i) instead of [i], and the using the autoware_ prefix in packages are very different in scope.

It would be helpful to explain why each item is included on this page clearly. For instance, essential points like using the autoware_ prefix should be clearly stated. On the other hand, for more specific details (like index access or logger usage), it might be better to phrase it as, “Here are some frequent issues raised in reviews or static analysis, referenced from the C++/ROS guidelines".

Thanks!! 👍
I will update document following @mitsudome-r after his answer to #599 (comment)

@mitsudome-r
Copy link
Member

@Shin-kyoto Thanks!

Move the content related to C++ and ROS to the separated pages.
Document Autoware-specific guidelines on this page.

I think this approach sounds reasonable to me.

…derstanding

Co-authored-by: Takamasa Horibe <horibe.takamasa@gmail.com>
@TakaHoribe TakaHoribe self-requested a review August 22, 2024 04:04
…guage and removed the content already covered in each language-specific guideline.

Signed-off-by: t4-adc <grp-rd-1-adc-admin@tier4.jp>
@Shin-kyoto
Copy link
Contributor Author

The guideline itself generally looks okay, but most of them seem to be duplicate of the materials in other pages like:

Could you explain the purpose of this change. Is your intention to add a summary page that require particular attention?

In that case, I'm okay to add Autoware specific guidelines mentioned in this summary page(like adding autoware_ prefix to package names), but I rather keep the details about C++ guidelines as a separate page and just put a link to that page. It would make us harder to keep consistency within our documentation if there are duplicate materials explained in different places.

@mitsudome-r @TakaHoribe @isamu-takagi
I changed in 289937d

@Shin-kyoto
Copy link
Contributor Author

@mitsudome-r @TakaHoribe
Can you review this PR, again?

Copy link
Member

@mitsudome-r mitsudome-r left a comment

Choose a reason for hiding this comment

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

I've made a small suggestions about the reference link. Otherwise, the PR looks good to me.

docs/contributing/coding-guidelines/index.md Show resolved Hide resolved
Copy link
Contributor

@TakaHoribe TakaHoribe left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for your work!

Signed-off-by: Shin-kyoto <58775300+Shin-kyoto@users.noreply.github.com>
@yukkysaito yukkysaito self-requested a review September 1, 2024 09:43
@Shin-kyoto
Copy link
Contributor Author

Shin-kyoto commented Sep 2, 2024

Thank you for your review!!
Contribution guideline is very important and it is necessary to review it strictly. Now, I confirmed that

  • I got the LGTMs from five awf members
  • I got the LGTM from maintainer @mitsudome-r

So, I will squash and merge it.

@Shin-kyoto Shin-kyoto merged commit 2f9b626 into autowarefoundation:main Sep 2, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tag:deploy-docs Mark for deploy-docs action generation. (used-by-ci) type:documentation Creating or refining documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants