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

Restructuring ROS 2 Gem docs #2266

Merged
merged 10 commits into from
Mar 20, 2023
Merged

Restructuring ROS 2 Gem docs #2266

merged 10 commits into from
Mar 20, 2023

Conversation

adamdbrw
Copy link
Contributor

@adamdbrw adamdbrw commented Feb 22, 2023

Change summary

Improving structure and information efficiency of ROS 2 Gem documentation.

This is a DRAFT. Description of changes will be updated.

Submission Checklist:

  • Descriptive active voice - Do descriptive sentences have a clear subject and action verb?
  • Answer the question at hand - Does the documentation answer a what, why, how, or where type of question?
  • Consistency - Does the content consistently follow the Style Guide?
  • Help the user - Does the documentation show the user something meaningful?

@adamdbrw adamdbrw requested review from a team as code owners February 22, 2023 16:09
@adamdbrw adamdbrw requested a review from willihay February 22, 2023 16:09
@adamdbrw adamdbrw changed the base branch from main to development February 22, 2023 16:09
@adamdbrw adamdbrw marked this pull request as draft February 22, 2023 16:09
@chanmosq chanmosq requested review from chanmosq and removed request for willihay February 22, 2023 16:18
@chanmosq
Copy link
Contributor

chanmosq commented Mar 8, 2023

@adamdbrw I've left some comments on the files. Here are some other changes to make:

  • Rename ros2 directory to robotics and _index.md to ros2.md The Gem Reference directories are purposely high-level categories. You would also need to update the link in gems/reference/_index.md. (This is different from the initial instruction I gave you, apologies for the setback)
  • Move class-diagram.md, concepts-and-components.md, and vehicle-dynamics.md to the subfolder interactivity/robotics. Only _index.md (soon-to-be ros2.md) should be in the gems/reference/robotics folder.
  • I made various comments to fix the linkTitle, title, and description to follow the metadata guidelines.

@chanmosq
Copy link
Contributor

chanmosq commented Mar 8, 2023

The DCO issue seems to point to 777ef82, a commit on this branch.
To resolve this, you can take one of the remediation steps outlined in the DCO check. (You can also find this page by clicking Details next to "DCO" at the bottom of a PR.)

I talked about this with @michalpelka as well and we've resolve the issue so this shouldn't happen anymore. In case this happens again for another team member, to summarize, the issue is because of different name and email configurations. The Git's web interface profile configuration is set to Michał Pełka <michalpelka@gmail.com> and the local git install configuration is set to Michał Pełka <michal.pelka@robotec.ai>. They must be exactly the same because that's how Git verifies that the committer is signed by the right person.

@adamdbrw adamdbrw force-pushed the restructuring-ros2-docs branch from 79eb9b0 to 06dab7e Compare March 10, 2023 15:20
@adamdbrw adamdbrw marked this pull request as ready for review March 10, 2023 15:21
@adamdbrw
Copy link
Contributor Author

All comments should be applied now. I think that the DCO issue resolved itself due to rebase (the culprit commit is actually not from the branch and the issue was resolved in upstream development). @chanmosq I also un-drafted the PR. @michalpelka could you review it as well and see if it works for you?

@adamdbrw adamdbrw requested a review from chanmosq March 13, 2023 09:51
Copy link
Contributor

@hultonha hultonha left a comment

Choose a reason for hiding this comment

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

This updates look great to me, awesome work @adamdbrw 🥳

chanmosq
chanmosq previously approved these changes Mar 13, 2023
@chanmosq chanmosq dismissed their stale review March 13, 2023 17:40

Remove approval for change to be made, but didn't request changes to avoid blocking

Copy link
Contributor

@wojtek-robotecai wojtek-robotecai left a comment

Choose a reason for hiding this comment

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

Take a look at my comments and suggestions

@adamdbrw
Copy link
Contributor Author

@chanmosq is there anything else we could do to proceed with the merge of this PR?

@chanmosq chanmosq added the do-not-merge/in-review Do not merge this PR. It is still being reviewed. label Mar 16, 2023
Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>
Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>
Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>
adamdbrw and others added 6 commits March 20, 2023 10:24
Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>
Signed-off-by: Michał Pełka <michal.pelka@robotec.ai>
Signed-off-by: Michał Pełka <michal.pelka@robotec.ai>
Co-authored-by: Wojtek <125894910+wojtek-robotecai@users.noreply.github.com>
Signed-off-by: Michał Pełka <michalpelka@gmail.com>
Signed-off-by: Michał Pełka <michal.pelka@robotec.ai>
Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>

Co-authored by: Channele Mosquera <chanmosq@amazon.com>
@adamdbrw adamdbrw force-pushed the restructuring-ros2-docs branch from e9a9d91 to 47fca24 Compare March 20, 2023 10:23
Signed-off-by: Adam Dabrowski <adam.dabrowski@robotec.ai>
@adamdbrw
Copy link
Contributor Author

@chanmosq I applied all the comments. I made some minor changes to reflect semantics.

@chanmosq
Copy link
Contributor

Looks great! Thanks @adamdbrw @michalpelka and @wojtek-robotecai

@chanmosq chanmosq added this pull request to the merge queue Mar 20, 2023
@chanmosq chanmosq merged commit f402e6f into development Mar 20, 2023
@chanmosq chanmosq deleted the restructuring-ros2-docs branch March 20, 2023 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/in-review Do not merge this PR. It is still being reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants