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

feat: add coding guideline about topic message handling #543

Conversation

ohsawa1204
Copy link
Contributor

@ohsawa1204 ohsawa1204 commented Apr 30, 2024

Description

Add coding guideline about topic message handling

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.

ohsawa1204 and others added 5 commits April 17, 2024 18:25
Signed-off-by: ohsawa1204 <ohsawa.12.04@gmail.com>
Signed-off-by: ohsawa1204 <ohsawa.12.04@gmail.com>
Signed-off-by: ohsawa1204 <ohsawa.12.04@gmail.com>
Signed-off-by: ohsawa1204 <ohsawa.12.04@gmail.com>
@ohsawa1204 ohsawa1204 marked this pull request as draft April 30, 2024 05:52
@ohsawa1204 ohsawa1204 changed the title Add coding guideline about topic message handling feat: add coding guideline about topic message handling Apr 30, 2024
ohsawa1204 and others added 2 commits April 30, 2024 15:15
@ohsawa1204 ohsawa1204 marked this pull request as ready for review April 30, 2024 06:19
@ohsawa1204 ohsawa1204 marked this pull request as draft April 30, 2024 06:19
@ohsawa1204 ohsawa1204 marked this pull request as ready for review April 30, 2024 06:34
@takam5f2 takam5f2 added the tag:deploy-docs Mark for deploy-docs action generation. (used-by-ci) label Apr 30, 2024
Signed-off-by: Takayuki AKAMINE <takayuki.akamine@tier4.jp>
@takam5f2
Copy link
Contributor

takam5f2 commented May 1, 2024

@ohsawa1204
I'd like to ask you to create a page structure before checking the content.
I'm concerned that there is no page index on the left-hand side of the web pages.

Please follow my instruction at first. If you find some improvements on my instruction, please fix it.


  1. add directory name of topic-message-handling to docs/contributing/coding-guidelines/ros-nodes/.pages
  2. add new docs/contributing/coding-guidelines/ros-nodes/topic-message-handling/.pages
nav:
  - index.md
  - 01-supp-intra-process-comm.md
  - 02-supp-waitset.md
  1. rename 00-topic-message-handling.md to index.md
  2. fix missing link to 00-topic-message-handling.md with replace it by index.md

I sent a PR (ohsawa1204#1) to your branch to show my instructions.
Best,

takam5f2 and others added 3 commits May 1, 2024 11:04
Signed-off-by: Takayuki AKAMINE <takayuki.akamine@tier4.jp>
docs(topic-message-handling): create page structure using .pages
Signed-off-by: ohsawa1204 <ohsawa.12.04@gmail.com>
@ohsawa1204
Copy link
Contributor Author

@ohsawa1204 I'd like to ask you to create a page structure before checking the content. I'm concerned that there is no page index on the left-hand side of the web pages.

Please follow my instruction at first. If you find some improvements on my instruction, please fix it.

  1. add directory name of topic-message-handling to docs/contributing/coding-guidelines/ros-nodes/.pages
  2. add new docs/contributing/coding-guidelines/ros-nodes/topic-message-handling/.pages
nav:
  - index.md
  - 01-supp-intra-process-comm.md
  - 02-supp-waitset.md
  1. rename 00-topic-message-handling.md to index.md
  2. fix missing link to 00-topic-message-handling.md with replace it by index.md

I sent a PR (ohsawa1204#1) to your branch to show my instructions. Best,

@takam5f2
Thank you very much for your fix!
Your fix was merged to this PR.

Signed-off-by: ohsawa1204 <ohsawa.12.04@gmail.com>
Copy link
Contributor

@takam5f2 takam5f2 left a comment

Choose a reason for hiding this comment

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

@ohsawa1204
Sorry for a large number of comments. I think that straight translation makes sentences complex, my feedback is based on this thought.

Copy link
Contributor

@takam5f2 takam5f2 left a comment

Choose a reason for hiding this comment

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

Sorry for my micro-checking.

Copy link
Contributor

@takam5f2 takam5f2 left a comment

Choose a reason for hiding this comment

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

@ohsawa1204
I finished checking all pages. Could you check my feedback?
Perhaps, some of my feedback may not reasonable.

Best,

ohsawa1204 and others added 5 commits May 2, 2024 15:18
…ndling/index.md

Co-authored-by: Takayuki AKAMINE <38586589+takam5f2@users.noreply.github.com>
…ndling/index.md

Co-authored-by: Takayuki AKAMINE <38586589+takam5f2@users.noreply.github.com>
…ndling/index.md

Co-authored-by: Takayuki AKAMINE <38586589+takam5f2@users.noreply.github.com>
…ndling/index.md

Co-authored-by: Takayuki AKAMINE <38586589+takam5f2@users.noreply.github.com>
…ndling/index.md

Co-authored-by: Takayuki AKAMINE <38586589+takam5f2@users.noreply.github.com>
ohsawa1204 and others added 7 commits May 14, 2024 11:55
…ndling/02-supp-wait_set.md

Co-authored-by: Takayuki AKAMINE <38586589+takam5f2@users.noreply.github.com>
…ndling/02-supp-wait_set.md

Co-authored-by: Takayuki AKAMINE <38586589+takam5f2@users.noreply.github.com>
…ndling/02-supp-wait_set.md

Co-authored-by: Takayuki AKAMINE <38586589+takam5f2@users.noreply.github.com>
…ndling/02-supp-wait_set.md

Co-authored-by: Takayuki AKAMINE <38586589+takam5f2@users.noreply.github.com>
…ndling/02-supp-wait_set.md

Co-authored-by: Takayuki AKAMINE <38586589+takam5f2@users.noreply.github.com>
…ndling/02-supp-wait_set.md

Co-authored-by: Takayuki AKAMINE <38586589+takam5f2@users.noreply.github.com>
…ndling/index.md

Co-authored-by: Takayuki AKAMINE <38586589+takam5f2@users.noreply.github.com>
Copy link
Contributor

@takam5f2 takam5f2 left a comment

Choose a reason for hiding this comment

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

@ohsawa1204 Thank you for checking all of my feedback. Do you have any action on this PR? If no, I'd like ask AWF reviewers to check this PR.

Best,

@ohsawa1204
Copy link
Contributor Author

@ohsawa1204 Thank you for checking all of my feedback. Do you have any action on this PR? If no, I'd like ask AWF reviewers to check this PR.

Best,

@takam5f2
No, I don't have any remaining action.

@takam5f2 takam5f2 requested a review from mitsudome-r May 14, 2024 06:42
@takam5f2
Copy link
Contributor

@mitsudome-r
@ohsawa1204 created the PR to add a new coding guidance using the Subscription->take() method. This was suggested in the discussion https://github.com/orgs/autowarefoundation/discussions/4612. The PR is ready to be reviewed.
Could you check the PR and invite other reviewers?
Best,

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.

The document generally looks good. I made few suggestions.

ohsawa1204 and others added 5 commits May 16, 2024 15:02
…ndling/index.md

Co-authored-by: Ryohsuke Mitsudome <43976834+mitsudome-r@users.noreply.github.com>
Signed-off-by: ohsawa1204 <ohsawa.12.04@gmail.com>
Signed-off-by: ohsawa1204 <ohsawa.12.04@gmail.com>
@mitsudome-r
Copy link
Member

@ohsawa1204 FYI, I have manually approved DCO check for this PR, but please sign-off your commits when you create a PR to Autoware repositories in the future. (for example with git commit -s)

Copy link
Contributor

@takam5f2 takam5f2 left a comment

Choose a reason for hiding this comment

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

@ohsawa1204
Thank you for your work.

@ohsawa1204
Copy link
Contributor Author

@ohsawa1204 FYI, I have manually approved DCO check for this PR, but please sign-off your commits when you create a PR to Autoware repositories in the future. (for example with git commit -s)

@mitsudome-r
I'm sorry for the inconvenience.
I will do so in the future.

@ohsawa1204
Copy link
Contributor Author

@ohsawa1204 Thank you for your work.

@takam5f2
Thank you for your time.
Thanks to you.

@ohsawa1204 ohsawa1204 merged commit cefcc1a into autowarefoundation:main May 17, 2024
9 of 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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants